GestureScreen: Skip screen properly.

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

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

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

@@ -28,9 +28,19 @@ constexpr const char kGestureBackPage[] = "gestureBack";
 
 }  // namespace
 
+// static
+std::string GestureNavigationScreen::GetResultString(Result result) {
+  switch (result) {
+    case Result::NEXT:
+      return "Next";
+    case Result::NOT_APPLICABLE:
+      return BaseScreen::kNotApplicable;
+  }
+}
+
 GestureNavigationScreen::GestureNavigationScreen(
     GestureNavigationScreenView* view,
-    const base::RepeatingClosure& exit_callback)
+    const ScreenExitCallback& exit_callback)
     : BaseScreen(GestureNavigationScreenView::kScreenId,
                  OobeScreenPriority::DEFAULT),
       view_(view),
@@ -50,7 +60,7 @@ void GestureNavigationScreen::GesturePageChange(const std::string& new_page) {
   current_page_ = new_page;
 }
 
-void GestureNavigationScreen::ShowImpl() {
+bool GestureNavigationScreen::MaybeSkip() {
   AccessibilityManager* accessibility_manager = AccessibilityManager::Get();
   if (chrome_user_manager_util::IsPublicSessionOrEphemeralLogin() ||
       !ash::features::IsHideShelfControlsInTabletModeEnabled() ||
@@ -59,18 +69,21 @@ void GestureNavigationScreen::ShowImpl() {
       accessibility_manager->IsSpokenFeedbackEnabled() ||
       accessibility_manager->IsAutoclickEnabled() ||
       accessibility_manager->IsSwitchAccessEnabled()) {
-    exit_callback_.Run();
-    return;
+    exit_callback_.Run(Result::NOT_APPLICABLE);
+    return true;
   }
 
   // Skip the screen if the device is not in tablet mode, unless tablet mode
   // first user run is forced on the device.
   if (!ash::TabletMode::Get()->InTabletMode() &&
       !chromeos::switches::ShouldOobeUseTabletModeFirstRun()) {
-    exit_callback_.Run();
-    return;
+    exit_callback_.Run(Result::NOT_APPLICABLE);
+    return true;
   }
+  return false;
+}
 
+void GestureNavigationScreen::ShowImpl() {
   // Begin keeping track of current page and start time for the page shown time
   // metrics.
   current_page_ = kGestureIntroPage;
@@ -91,7 +104,7 @@ void GestureNavigationScreen::OnUserAction(const std::string& action_id) {
         ash::prefs::kGestureEducationNotificationShown, true);
 
     RecordPageShownTimeMetrics();
-    exit_callback_.Run();
+    exit_callback_.Run(Result::NEXT);
   } else {
     BaseScreen::OnUserAction(action_id);
   }

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

@@ -17,21 +17,32 @@ namespace chromeos {
 // The OOBE screen dedicated to gesture navigation education.
 class GestureNavigationScreen : public BaseScreen {
  public:
+  enum class Result { NEXT, NOT_APPLICABLE };
+
+  static std::string GetResultString(Result result);
+
+  using ScreenExitCallback = base::RepeatingCallback<void(Result result)>;
   GestureNavigationScreen(GestureNavigationScreenView* view,
-                          const base::RepeatingClosure& exit_callback);
+                          const ScreenExitCallback& exit_callback);
   ~GestureNavigationScreen() override;
 
   GestureNavigationScreen(const GestureNavigationScreen&) = delete;
   GestureNavigationScreen operator=(const GestureNavigationScreen&) = delete;
 
-  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_;
+  }
+
   // Called when the currently shown page is changed.
   void GesturePageChange(const std::string& new_page);
 
+  // BaseScreen:
+  bool MaybeSkip() override;
+
  protected:
   // BaseScreen:
   void ShowImpl() override;
@@ -43,7 +54,7 @@ class GestureNavigationScreen : public BaseScreen {
   void RecordPageShownTimeMetrics();
 
   GestureNavigationScreenView* view_;
-  base::RepeatingClosure exit_callback_;
+  ScreenExitCallback exit_callback_;
 
   // Used to keep track of the current elapsed time that each page has been
   // shown for.

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

@@ -54,10 +54,10 @@ class GestureNavigationScreenTest
         static_cast<GestureNavigationScreen*>(
             WizardController::default_controller()->screen_manager()->GetScreen(
                 GestureNavigationScreenView::kScreenId));
+    original_callback_ = gesture_screen->get_exit_callback_for_testing();
     gesture_screen->set_exit_callback_for_testing(
         base::BindRepeating(&GestureNavigationScreenTest::HandleScreenExit,
                             base::Unretained(this)));
-
     OobeBaseTest::SetUpOnMainThread();
   }
 
@@ -106,14 +106,19 @@ class GestureNavigationScreenTest
     run_loop.Run();
   }
 
+  base::Optional<GestureNavigationScreen::Result> screen_result_;
+  base::HistogramTester histogram_tester_;
+
  private:
-  void HandleScreenExit() {
+  void HandleScreenExit(GestureNavigationScreen::Result result) {
     ASSERT_FALSE(screen_exited_);
     screen_exited_ = true;
+    screen_result_ = result;
+    original_callback_.Run(result);
     if (screen_exit_callback_)
       std::move(screen_exit_callback_).Run();
   }
-
+  GestureNavigationScreen::ScreenExitCallback original_callback_;
   bool screen_exited_ = false;
   base::RepeatingClosure screen_exit_callback_;
   base::test::ScopedFeatureList feature_list_;
@@ -166,6 +171,11 @@ IN_PROC_BROWSER_TEST_P(GestureNavigationScreenTest, FlowTest) {
   test::OobeJS().TapOnPath({"gesture-navigation", "gesture-back-next-button"});
 
   WaitForScreenExit();
+  EXPECT_EQ(screen_result_.value(), GestureNavigationScreen::Result::NEXT);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTimeByExitReason.Gesture-navigation.Next", 1);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTime.Gesture-navigation", 1);
 }
 
 // Ensure the flow is skipped when in clamshell mode.
@@ -176,6 +186,8 @@ IN_PROC_BROWSER_TEST_P(GestureNavigationScreenTest, ScreenSkippedInClamshell) {
 
   if (ShouldBeSkippedInClamshell()) {
     WaitForScreenExit();
+    EXPECT_EQ(screen_result_.value(),
+              GestureNavigationScreen::Result::NOT_APPLICABLE);
   } else {
     OobeScreenWaiter(GestureNavigationScreenView::kScreenId).Wait();
   }
@@ -189,6 +201,12 @@ IN_PROC_BROWSER_TEST_P(GestureNavigationScreenTest,
   ShowGestureNavigationScreen();
 
   WaitForScreenExit();
+  EXPECT_EQ(screen_result_.value(),
+            GestureNavigationScreen::Result::NOT_APPLICABLE);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTimeByExitReason.Gesture-navigation.Next", 0);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTime.Gesture-navigation", 0);
 }
 
 // Ensure the flow is skipped when autoclick is enabled.
@@ -199,6 +217,12 @@ IN_PROC_BROWSER_TEST_P(GestureNavigationScreenTest,
   ShowGestureNavigationScreen();
 
   WaitForScreenExit();
+  EXPECT_EQ(screen_result_.value(),
+            GestureNavigationScreen::Result::NOT_APPLICABLE);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTimeByExitReason.Gesture-navigation.Next", 0);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTime.Gesture-navigation", 0);
 }
 
 // Ensure the flow is skipped when switch access is enabled.
@@ -209,6 +233,12 @@ IN_PROC_BROWSER_TEST_P(GestureNavigationScreenTest,
   ShowGestureNavigationScreen();
 
   WaitForScreenExit();
+  EXPECT_EQ(screen_result_.value(),
+            GestureNavigationScreen::Result::NOT_APPLICABLE);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTimeByExitReason.Gesture-navigation.Next", 0);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTime.Gesture-navigation", 0);
 }
 
 // Ensure the flow is skipped when shelf navigation buttons are enabled.
@@ -220,13 +250,17 @@ IN_PROC_BROWSER_TEST_P(GestureNavigationScreenTest,
   ShowGestureNavigationScreen();
 
   WaitForScreenExit();
+  EXPECT_EQ(screen_result_.value(),
+            GestureNavigationScreen::Result::NOT_APPLICABLE);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTimeByExitReason.Gesture-navigation.Next", 0);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTime.Gesture-navigation", 0);
 }
 
 // Ensure the page shown time metrics are being recorded during the gesture
 // navigation screen flow
 IN_PROC_BROWSER_TEST_P(GestureNavigationScreenTest, PageShownMetricsTest) {
-  base::HistogramTester histogram_tester_;
-
   ShowGestureNavigationScreen();
   OobeScreenWaiter(GestureNavigationScreenView::kScreenId).Wait();
 
@@ -244,6 +278,7 @@ IN_PROC_BROWSER_TEST_P(GestureNavigationScreenTest, PageShownMetricsTest) {
   test::OobeJS().TapOnPath({"gesture-navigation", "gesture-back-next-button"});
 
   WaitForScreenExit();
+  EXPECT_EQ(screen_result_.value(), GestureNavigationScreen::Result::NEXT);
 
   histogram_tester_.ExpectTotalCount(
       "OOBE.GestureNavigationScreen.PageShownTime.Intro", 1);
@@ -253,6 +288,10 @@ IN_PROC_BROWSER_TEST_P(GestureNavigationScreenTest, PageShownMetricsTest) {
       "OOBE.GestureNavigationScreen.PageShownTime.Overview", 1);
   histogram_tester_.ExpectTotalCount(
       "OOBE.GestureNavigationScreen.PageShownTime.Back", 1);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTimeByExitReason.Gesture-navigation.Next", 1);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTime.Gesture-navigation", 1);
 }
 
 }  // namespace chromeos

chrome/browser/chromeos/login/wizard_controller.cc

@@ -1200,8 +1200,10 @@ void WizardController::OnMultiDeviceSetupScreenExit() {
   ShowGestureNavigationScreen();
 }
 
-void WizardController::OnGestureNavigationScreenExit() {
-  OnScreenExit(GestureNavigationScreenView::kScreenId, kDefaultExitReason);
+void WizardController::OnGestureNavigationScreenExit(
+    GestureNavigationScreen::Result result) {
+  OnScreenExit(GestureNavigationScreenView::kScreenId,
+               GestureNavigationScreen::GetResultString(result));
 
   ShowMarketingOptInScreen();
 }

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/gesture_navigation_screen.h"
 #include "chrome/browser/chromeos/login/screens/kiosk_autolaunch_screen.h"
 #include "chrome/browser/chromeos/login/screens/network_screen.h"
 #include "chrome/browser/chromeos/login/screens/packaged_license_screen.h"
@@ -248,7 +249,7 @@ class WizardController {
   void OnAppDownloadingScreenExit();
   void OnAssistantOptInFlowScreenExit();
   void OnMultiDeviceSetupScreenExit();
-  void OnGestureNavigationScreenExit();
+  void OnGestureNavigationScreenExit(GestureNavigationScreen::Result result);
   void OnMarketingOptInScreenExit();
   void OnResetScreenExit();
   void OnDeviceModificationCanceled();