DiscoverScreen: Skip screen properly.

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

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

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

@@ -19,8 +19,18 @@ namespace {
 const char kFinished[] = "finished";
 }
 
+// static
+std::string DiscoverScreen::GetResultString(Result result) {
+  switch (result) {
+    case Result::NEXT:
+      return "Next";
+    case Result::NOT_APPLICABLE:
+      return BaseScreen::kNotApplicable;
+  }
+}
+
 DiscoverScreen::DiscoverScreen(DiscoverScreenView* view,
-                               const base::RepeatingClosure& exit_callback)
+                               const ScreenExitCallback& exit_callback)
     : BaseScreen(DiscoverScreenView::kScreenId, OobeScreenPriority::DEFAULT),
       view_(view),
       exit_callback_(exit_callback) {
@@ -32,24 +42,29 @@ DiscoverScreen::~DiscoverScreen() {
   view_->Bind(nullptr);
 }
 
-void DiscoverScreen::ShowImpl() {
+bool DiscoverScreen::MaybeSkip() {
   PrefService* prefs = ProfileManager::GetActiveUserProfile()->GetPrefs();
   if (chrome_user_manager_util::IsPublicSessionOrEphemeralLogin() ||
       !chromeos::quick_unlock::IsPinEnabled(prefs) ||
       chromeos::quick_unlock::IsPinDisabledByPolicy(prefs)) {
-    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;
   }
 
-  view_->Show();
+  return false;
+}
+
+void DiscoverScreen::ShowImpl() {
+  if (view_)
+    view_->Show();
 }
 
 void DiscoverScreen::HideImpl() {
@@ -59,7 +74,7 @@ void DiscoverScreen::HideImpl() {
 void DiscoverScreen::OnUserAction(const std::string& action_id) {
   // Only honor finish if discover is currently being shown.
   if (action_id == kFinished) {
-    exit_callback_.Run();
+    exit_callback_.Run(Result::NEXT);
     return;
   }
   BaseScreen::OnUserAction(action_id);

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

@@ -17,19 +17,33 @@ class DiscoverScreenView;
 
 class DiscoverScreen : public BaseScreen {
  public:
+  enum class Result { NEXT, NOT_APPLICABLE };
+
+  static std::string GetResultString(Result result);
+
+  using ScreenExitCallback = base::RepeatingCallback<void(Result result)>;
   DiscoverScreen(DiscoverScreenView* view,
-                 const base::RepeatingClosure& exit_callback);
+                 const ScreenExitCallback& exit_callback);
   ~DiscoverScreen() override;
 
+  void set_exit_callback_for_testing(const ScreenExitCallback& exit_callback) {
+    exit_callback_ = exit_callback;
+  }
+
+  const ScreenExitCallback& get_exit_callback_for_testing() {
+    return exit_callback_;
+  }
+
  protected:
   // BaseScreen:
+  bool MaybeSkip() override;
   void ShowImpl() override;
   void HideImpl() override;
   void OnUserAction(const std::string& action_id) override;
 
  private:
   DiscoverScreenView* const view_;
-  base::RepeatingClosure exit_callback_;
+  ScreenExitCallback exit_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(DiscoverScreen);
 };

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

@@ -0,0 +1,111 @@
+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/chromeos/login/screens/discover_screen.h"
+
+#include "ash/public/cpp/test/shell_test_api.h"
+#include "base/bind.h"
+#include "base/run_loop.h"
+#include "chrome/browser/chromeos/login/screen_manager.h"
+#include "chrome/browser/chromeos/login/test/js_checker.h"
+#include "chrome/browser/chromeos/login/test/login_manager_mixin.h"
+#include "chrome/browser/chromeos/login/test/oobe_base_test.h"
+#include "chrome/browser/chromeos/login/test/oobe_screen_exit_waiter.h"
+#include "chrome/browser/chromeos/login/test/oobe_screen_waiter.h"
+#include "chrome/browser/chromeos/login/ui/login_display_host.h"
+#include "chrome/browser/chromeos/login/wizard_controller.h"
+#include "chrome/browser/ui/webui/chromeos/login/discover_screen_handler.h"
+#include "chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h"
+#include "chromeos/constants/chromeos_features.h"
+#include "content/public/test/browser_test.h"
+
+namespace chromeos {
+
+class DiscoverScreenTest : public OobeBaseTest {
+ public:
+  DiscoverScreenTest() {
+    // To reuse existing wizard controller in the flow.
+    feature_list_.InitAndEnableFeature(
+        chromeos::features::kOobeScreensPriority);
+  }
+  ~DiscoverScreenTest() override = default;
+
+  void SetUpOnMainThread() override {
+    DiscoverScreen* screen = static_cast<DiscoverScreen*>(
+        WizardController::default_controller()->screen_manager()->GetScreen(
+            DiscoverScreenView::kScreenId));
+    original_callback_ = screen->get_exit_callback_for_testing();
+    screen->set_exit_callback_for_testing(base::BindRepeating(
+        &DiscoverScreenTest::HandleScreenExit, base::Unretained(this)));
+
+    OobeBaseTest::SetUpOnMainThread();
+  }
+
+  void ShowDiscoverScreen() {
+    login_manager_mixin_.LoginAsNewReguarUser();
+    OobeScreenExitWaiter(GaiaView::kScreenId).Wait();
+    if (!screen_exited_) {
+      LoginDisplayHost::default_host()->StartWizard(
+          DiscoverScreenView::kScreenId);
+    }
+  }
+
+  void WaitForScreenShown() {
+    OobeScreenWaiter(DiscoverScreenView::kScreenId).Wait();
+  }
+
+  void WaitForScreenExit() {
+    if (screen_exited_)
+      return;
+    base::RunLoop run_loop;
+    screen_exit_callback_ = run_loop.QuitClosure();
+    run_loop.Run();
+  }
+
+  base::Optional<DiscoverScreen::Result> screen_result_;
+  base::HistogramTester histogram_tester_;
+
+ private:
+  void HandleScreenExit(DiscoverScreen::Result result) {
+    screen_exited_ = true;
+    screen_result_ = result;
+    original_callback_.Run(result);
+    if (screen_exit_callback_)
+      std::move(screen_exit_callback_).Run();
+  }
+
+  base::test::ScopedFeatureList feature_list_;
+  DiscoverScreen::ScreenExitCallback original_callback_;
+  bool screen_exited_ = false;
+  base::RepeatingClosure screen_exit_callback_;
+
+  LoginManagerMixin login_manager_mixin_{&mixin_host_};
+};
+
+IN_PROC_BROWSER_TEST_F(DiscoverScreenTest, Skipped) {
+  ShowDiscoverScreen();
+
+  WaitForScreenExit();
+  EXPECT_EQ(screen_result_.value(), DiscoverScreen::Result::NOT_APPLICABLE);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTimeByExitReason.Discover.Next", 0);
+  histogram_tester_.ExpectTotalCount("OOBE.StepCompletionTime.Discover", 0);
+}
+
+IN_PROC_BROWSER_TEST_F(DiscoverScreenTest, BasicFlow) {
+  ash::ShellTestApi().SetTabletModeEnabledForTest(true);
+  ShowDiscoverScreen();
+  WaitForScreenShown();
+
+  test::OobeJS().TapOnPath(
+      {"discover-impl", "pin-setup-impl", "setupSkipButton"});
+
+  WaitForScreenExit();
+  EXPECT_EQ(screen_result_.value(), DiscoverScreen::Result::NEXT);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTimeByExitReason.Discover.Next", 1);
+  histogram_tester_.ExpectTotalCount("OOBE.StepCompletionTime.Discover", 1);
+}
+
+}  // namespace chromeos

chrome/browser/chromeos/login/wizard_controller.cc

@@ -1107,8 +1107,10 @@ void WizardController::OnFingerprintSetupScreenExit(
   ShowDiscoverScreen();
 }
 
-void WizardController::OnDiscoverScreenExit() {
-  OnScreenExit(DiscoverScreenView::kScreenId, kDefaultExitReason);
+void WizardController::OnDiscoverScreenExit(DiscoverScreen::Result result) {
+  OnScreenExit(DiscoverScreenView::kScreenId,
+               DiscoverScreen::GetResultString(result));
+
   ShowArcTermsOfServiceScreen();
 }
 

chrome/browser/chromeos/login/wizard_controller.h

@@ -26,6 +26,7 @@
 #include "chrome/browser/chromeos/login/screens/assistant_optin_flow_screen.h"
 #include "chrome/browser/chromeos/login/screens/demo_preferences_screen.h"
 #include "chrome/browser/chromeos/login/screens/demo_setup_screen.h"
+#include "chrome/browser/chromeos/login/screens/discover_screen.h"
 #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"
@@ -233,7 +234,7 @@ class WizardController {
   void OnTermsOfServiceScreenExit(TermsOfServiceScreen::Result result);
   void OnFingerprintSetupScreenExit(FingerprintSetupScreen::Result result);
   void OnSyncConsentScreenExit(SyncConsentScreen::Result result);
-  void OnDiscoverScreenExit();
+  void OnDiscoverScreenExit(DiscoverScreen::Result result);
   void OnArcTermsOfServiceScreenExit(ArcTermsOfServiceScreen::Result result);
   void OnArcTermsOfServiceAccepted();
   void OnRecommendAppsScreenExit(RecommendAppsScreen::Result result);

chrome/browser/resources/chromeos/login/discover/discover_ui.html

@@ -43,7 +43,7 @@
           module="setWallpaper">
       </discover-set-wallpaper-card>
     </discover-welcome>
-    <discover-pin-setup-module class="module"
+    <discover-pin-setup-module class="module" id="pin-setup-impl"
         module="pinSetup" first-run="[[firstRun]]" hidden>
     </discover-pin-setup-module>
   </template>

chrome/test/BUILD.gn

@@ -2228,6 +2228,7 @@ if (!is_android) {
         "../browser/chromeos/login/saml/security_token_saml_browsertest.cc",
         "../browser/chromeos/login/screens/app_downloading_screen_browsertest.cc",
         "../browser/chromeos/login/screens/assistant_optin_flow_screen_browsertest.cc",
+        "../browser/chromeos/login/screens/discover_screen_browsertest.cc",
         "../browser/chromeos/login/screens/fingerprint_setup_browsertest.cc",
         "../browser/chromeos/login/screens/gesture_navigation_screen_browsertest.cc",
         "../browser/chromeos/login/screens/hid_detection_screen_browsertest.cc",

testing/buildbot/filters/chromeos.msan.browser_tests.oobe_negative.filter

@@ -26,6 +26,7 @@
 -DeviceDisablingBeforeLoginHostCreated.*
 -DeviceDisablingTest.*
 -DeviceIDTest.*
+-DiscoverScreenTest.*
 -DockedMagnifierVirtualKeyboardTest.*
 -EnableDebuggingDevTest.*
 -EnableDebuggingNonDevTest.*

testing/buildbot/filters/chromeos.msan.browser_tests.oobe_positive.filter

@@ -26,6 +26,7 @@ DemoSetupTestBase.*
 DeviceDisablingBeforeLoginHostCreated.*
 DeviceDisablingTest.*
 DeviceIDTest.*
+DiscoverScreenTest.*
 DockedMagnifierVirtualKeyboardTest.*
 EnableDebuggingDevTest.*
 EnableDebuggingNonDevTest.*