MultiDeviceSetupScreen: Skip screen properly.

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

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

chrome/browser/chromeos/BUILD.gn

@@ -3205,7 +3205,6 @@ source_set("unit_tests") {
     "login/saml/mock_lock_handler.h",
     "login/saml/password_expiry_notification_unittest.cc",
     "login/saml/saml_offline_signin_limiter_unittest.cc",
-    "login/screens/multidevice_setup_screen_unittest.cc",
     "login/screens/network_screen_unittest.cc",
     "login/screens/recommend_apps/recommend_apps_fetcher_impl_unittest.cc",
     "login/screens/update_required_screen_unittest.cc",

chrome/browser/chromeos/login/arc_terms_of_service_browsertest.cc

@@ -724,7 +724,6 @@ class PublicAccountArcTermsOfServiceScreenTest
 IN_PROC_BROWSER_TEST_F(PublicAccountArcTermsOfServiceScreenTest,
                        SkippedForPublicAccount) {
   StartPublicSession();
-  ShowArcTosScreen();
 
   chromeos::test::WaitForPrimaryUserSessionStart();
   histogram_tester_.ExpectTotalCount(

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

@@ -24,9 +24,19 @@ constexpr const char kDeclinedSetupUserAction[] = "setup-declined";
 
 }  // namespace
 
+// static
+std::string MultiDeviceSetupScreen::GetResultString(Result result) {
+  switch (result) {
+    case Result::NEXT:
+      return "Next";
+    case Result::NOT_APPLICABLE:
+      return BaseScreen::kNotApplicable;
+  }
+}
+
 MultiDeviceSetupScreen::MultiDeviceSetupScreen(
     MultiDeviceSetupScreenView* view,
-    const base::RepeatingClosure& exit_callback)
+    const ScreenExitCallback& exit_callback)
     : BaseScreen(MultiDeviceSetupScreenView::kScreenId,
                  OobeScreenPriority::DEFAULT),
       view_(view),
@@ -39,32 +49,40 @@ MultiDeviceSetupScreen::~MultiDeviceSetupScreen() {
   view_->Bind(nullptr);
 }
 
-void MultiDeviceSetupScreen::ShowImpl() {
-  // Only attempt the setup flow for non-guest users.
-  if (chrome_user_manager_util::IsPublicSessionOrEphemeralLogin()) {
-    ExitScreen();
-    return;
+void MultiDeviceSetupScreen::TryInitSetupClient() {
+  if (!setup_client_) {
+    setup_client_ =
+        multidevice_setup::MultiDeviceSetupClientFactory::GetForProfile(
+            ProfileManager::GetActiveUserProfile());
   }
+}
 
-  multidevice_setup::MultiDeviceSetupClient* client =
-      multidevice_setup::MultiDeviceSetupClientFactory::GetForProfile(
-          ProfileManager::GetActiveUserProfile());
-
-  if (!client) {
-    ExitScreen();
-    return;
+bool MultiDeviceSetupScreen::MaybeSkip(WizardContext* /*context*/) {
+  // Only attempt the setup flow for non-guest users.
+  if (chrome_user_manager_util::IsPublicSessionOrEphemeralLogin()) {
+    exit_callback_.Run(Result::NOT_APPLICABLE);
+    return true;
   }
 
+  TryInitSetupClient();
   // If there is no eligible multi-device host phone or if there is a phone and
   // it has already been set, skip the setup flow.
-  if (client->GetHostStatus().first !=
+  if (!setup_client_) {
+    exit_callback_.Run(Result::NOT_APPLICABLE);
+    return true;
+  }
+  if (setup_client_->GetHostStatus().first !=
       multidevice_setup::mojom::HostStatus::kEligibleHostExistsButNoHostSet) {
     VLOG(1) << "Skipping MultiDevice setup screen; host status: "
-            << client->GetHostStatus().first;
-    ExitScreen();
-    return;
+            << setup_client_->GetHostStatus().first;
+    exit_callback_.Run(Result::NOT_APPLICABLE);
+    return true;
   }
 
+  return false;
+}
+
+void MultiDeviceSetupScreen::ShowImpl() {
   view_->Show();
 
   // Record that user was presented with setup flow to prevent spam
@@ -84,11 +102,11 @@ void MultiDeviceSetupScreen::OnUserAction(const std::string& action_id) {
   if (action_id == kAcceptedSetupUserAction) {
     RecordMultiDeviceSetupOOBEUserChoiceHistogram(
         MultiDeviceSetupOOBEUserChoice::kAccepted);
-    ExitScreen();
+    exit_callback_.Run(Result::NEXT);
   } else if (action_id == kDeclinedSetupUserAction) {
     RecordMultiDeviceSetupOOBEUserChoiceHistogram(
         MultiDeviceSetupOOBEUserChoice::kDeclined);
-    ExitScreen();
+    exit_callback_.Run(Result::NEXT);
   } else {
     BaseScreen::OnUserAction(action_id);
     NOTREACHED();
@@ -100,8 +118,4 @@ void MultiDeviceSetupScreen::RecordMultiDeviceSetupOOBEUserChoiceHistogram(
   UMA_HISTOGRAM_ENUMERATION("MultiDeviceSetup.OOBE.UserChoice", value);
 }
 
-void MultiDeviceSetupScreen::ExitScreen() {
-  exit_callback_.Run();
-}
-
 }  // namespace chromeos

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

@@ -7,22 +7,50 @@
 
 #include <string>
 
+#include "base/bind.h"
 #include "base/callback.h"
 #include "base/macros.h"
 #include "chrome/browser/chromeos/login/screens/base_screen.h"
 
 namespace chromeos {
 
+namespace multidevice_setup {
+
+class MultiDeviceSetupClient;
+
+}  // namespace multidevice_setup
+
 class MultiDeviceSetupScreenView;
 
 class MultiDeviceSetupScreen : public BaseScreen {
  public:
+  enum class Result { NEXT, NOT_APPLICABLE };
+
+  static std::string GetResultString(Result result);
+
+  using ScreenExitCallback = base::RepeatingCallback<void(Result result)>;
   MultiDeviceSetupScreen(MultiDeviceSetupScreenView* view,
-                         const base::RepeatingClosure& exit_callback);
+                         const ScreenExitCallback& exit_callback);
   ~MultiDeviceSetupScreen() override;
 
+  void AddExitCallbackForTesting(const ScreenExitCallback& testing_callback) {
+    exit_callback_ = base::BindRepeating(
+        [](const ScreenExitCallback& original_callback,
+           const ScreenExitCallback& testing_callback, Result result) {
+          original_callback.Run(result);
+          testing_callback.Run(result);
+        },
+        exit_callback_, testing_callback);
+  }
+
+  void set_multidevice_setup_client_for_testing(
+      multidevice_setup::MultiDeviceSetupClient* client) {
+    setup_client_ = client;
+  }
+
  protected:
   // BaseScreen:
+  bool MaybeSkip(WizardContext* context) override;
   void ShowImpl() override;
   void HideImpl() override;
   void OnUserAction(const std::string& action_id) override;
@@ -40,14 +68,16 @@ class MultiDeviceSetupScreen : public BaseScreen {
     kMaxValue = kDeclined
   };
 
+  // Inits |setup_client_| if it was not initialized before.
+  void TryInitSetupClient();
+
   static void RecordMultiDeviceSetupOOBEUserChoiceHistogram(
       MultiDeviceSetupOOBEUserChoice value);
 
-  // Exits the screen.
-  void ExitScreen();
+  multidevice_setup::MultiDeviceSetupClient* setup_client_ = nullptr;
 
   MultiDeviceSetupScreenView* view_;
-  base::RepeatingClosure exit_callback_;
+  ScreenExitCallback exit_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(MultiDeviceSetupScreen);
 };

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

+// 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/multidevice_setup_screen.h"
+
+#include "base/bind.h"
+#include "base/run_loop.h"
+#include "chrome/browser/browser_process.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/gaia_screen_handler.h"
+#include "chrome/browser/ui/webui/chromeos/login/multidevice_setup_screen_handler.h"
+#include "chromeos/constants/chromeos_features.h"
+#include "chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup_client.h"
+#include "content/public/test/browser_test.h"
+
+namespace chromeos {
+
+class MultiDeviceSetupScreenTest : public OobeBaseTest {
+ public:
+  MultiDeviceSetupScreenTest() {
+    // To reuse existing wizard controller in the flow.
+    feature_list_.InitAndEnableFeature(
+        chromeos::features::kOobeScreensPriority);
+  }
+  ~MultiDeviceSetupScreenTest() override = default;
+
+  void SetUpOnMainThread() override {
+    MultiDeviceSetupScreen* screen = static_cast<MultiDeviceSetupScreen*>(
+        WizardController::default_controller()->screen_manager()->GetScreen(
+            MultiDeviceSetupScreenView::kScreenId));
+    screen->AddExitCallbackForTesting(base::BindRepeating(
+        &MultiDeviceSetupScreenTest::HandleScreenExit, base::Unretained(this)));
+
+    fake_multidevice_setup_client_ =
+        std::make_unique<multidevice_setup::FakeMultiDeviceSetupClient>();
+    screen->set_multidevice_setup_client_for_testing(
+        fake_multidevice_setup_client_.get());
+    OobeBaseTest::SetUpOnMainThread();
+  }
+
+  void SimulateHostStatusChange() {
+    multidevice_setup::MultiDeviceSetupClient::HostStatusWithDevice
+        host_status_with_device = multidevice_setup::MultiDeviceSetupClient::
+            GenerateDefaultHostStatusWithDevice();
+    host_status_with_device.first =
+        multidevice_setup::mojom::HostStatus::kEligibleHostExistsButNoHostSet;
+    fake_multidevice_setup_client_->SetHostStatusWithDevice(
+        host_status_with_device);
+  }
+
+  void ShowMultiDeviceSetupScreen() {
+    login_manager_mixin_.LoginAsNewRegularUser();
+    OobeScreenExitWaiter(GaiaView::kScreenId).Wait();
+  }
+
+  void FinishDeviceSetup() {
+    test::OobeJS().Evaluate(
+        R"($('multidevice-setup-impl')
+          .$['multideviceSetup']
+          .fire('setup-exited', {didUserCompleteSetup: true});)");
+  }
+
+  void CancelDeviceSetup() {
+    test::OobeJS().Evaluate(
+        R"($('multidevice-setup-impl')
+          .$['multideviceSetup']
+          .fire('setup-exited', {didUserCompleteSetup: false});)");
+  }
+
+  void WaitForScreenShown() {
+    OobeScreenWaiter(MultiDeviceSetupScreenView::kScreenId).Wait();
+  }
+
+  void WaitForScreenExit() {
+    if (screen_exited_)
+      return;
+    base::RunLoop run_loop;
+    screen_exit_callback_ = run_loop.QuitClosure();
+    run_loop.Run();
+  }
+
+  void CheckUserChoice(bool Accepted) {
+    histogram_tester_.ExpectBucketCount<
+        MultiDeviceSetupScreen::MultiDeviceSetupOOBEUserChoice>(
+        "MultiDeviceSetup.OOBE.UserChoice",
+        MultiDeviceSetupScreen::MultiDeviceSetupOOBEUserChoice::kAccepted,
+        Accepted);
+    histogram_tester_.ExpectBucketCount<
+        MultiDeviceSetupScreen::MultiDeviceSetupOOBEUserChoice>(
+        "MultiDeviceSetup.OOBE.UserChoice",
+        MultiDeviceSetupScreen::MultiDeviceSetupOOBEUserChoice::kDeclined,
+        !Accepted);
+  }
+
+  base::Optional<MultiDeviceSetupScreen::Result> screen_result_;
+  base::HistogramTester histogram_tester_;
+
+ private:
+  void HandleScreenExit(MultiDeviceSetupScreen::Result result) {
+    ASSERT_FALSE(screen_exited_);
+    screen_exited_ = true;
+    screen_result_ = result;
+    if (screen_exit_callback_)
+      std::move(screen_exit_callback_).Run();
+  }
+
+  bool screen_exited_ = false;
+  base::RepeatingClosure screen_exit_callback_;
+
+  base::test::ScopedFeatureList feature_list_;
+  std::unique_ptr<multidevice_setup::FakeMultiDeviceSetupClient>
+      fake_multidevice_setup_client_;
+
+  LoginManagerMixin login_manager_mixin_{&mixin_host_};
+};
+
+IN_PROC_BROWSER_TEST_F(MultiDeviceSetupScreenTest, Accepted) {
+  SimulateHostStatusChange();
+  ShowMultiDeviceSetupScreen();
+  WaitForScreenShown();
+
+  FinishDeviceSetup();
+
+  WaitForScreenExit();
+  EXPECT_EQ(screen_result_.value(), MultiDeviceSetupScreen::Result::NEXT);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTimeByExitReason.Multidevice-setup.Next", 1);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTime.Multidevice-setup", 1);
+  CheckUserChoice(true);
+}
+
+IN_PROC_BROWSER_TEST_F(MultiDeviceSetupScreenTest, Declined) {
+  SimulateHostStatusChange();
+  ShowMultiDeviceSetupScreen();
+  WaitForScreenShown();
+
+  CancelDeviceSetup();
+
+  WaitForScreenExit();
+  EXPECT_EQ(screen_result_.value(), MultiDeviceSetupScreen::Result::NEXT);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTimeByExitReason.Multidevice-setup.Next", 1);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTime.Multidevice-setup", 1);
+  CheckUserChoice(false);
+}
+
+IN_PROC_BROWSER_TEST_F(MultiDeviceSetupScreenTest, Skipped) {
+  ShowMultiDeviceSetupScreen();
+
+  WaitForScreenExit();
+  EXPECT_EQ(screen_result_.value(),
+            MultiDeviceSetupScreen::Result::NOT_APPLICABLE);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTimeByExitReason.Multidevice-setup.Next", 0);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTime.Multidevice-setup", 0);
+}
+
+}  // namespace chromeos

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

-// Copyright 2018 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/multidevice_setup_screen.h"
-
-#include <memory>
-
-#include "base/bind_helpers.h"
-#include "base/test/metrics/histogram_tester.h"
-#include "chrome/browser/ui/webui/chromeos/login/multidevice_setup_screen_handler.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace chromeos {
-
-namespace {
-
-class FakeMultiDeviceSetupScreenView : public MultiDeviceSetupScreenView {
- public:
-  FakeMultiDeviceSetupScreenView() = default;
-  ~FakeMultiDeviceSetupScreenView() override = default;
-
-  // MultiDeviceSetupScreenView:
-  void Bind(MultiDeviceSetupScreen* screen) override {}
-  void Show() override {}
-  void Hide() override {}
-};
-
-}  // namespace
-
-class MultiDeviceSetupScreenTest : public testing::Test {
- public:
-  MultiDeviceSetupScreenTest() = default;
-  ~MultiDeviceSetupScreenTest() override = default;
-
-  // testing::Test:
-  void SetUp() override {
-    multi_device_setup_screen_ = std::make_unique<MultiDeviceSetupScreen>(
-        &fake_multi_device_setup_screen_view_, base::DoNothing());
-  }
-
-  void TearDown() override {}
-
-  std::unique_ptr<MultiDeviceSetupScreen> multi_device_setup_screen_;
-
-  void VerifyUserChoicePaths() {
-    histogram_tester_.ExpectTotalCount("MultiDeviceSetup.OOBE.UserChoice", 0);
-
-    multi_device_setup_screen_->OnUserAction("setup-accepted");
-
-    histogram_tester_.ExpectBucketCount<
-        MultiDeviceSetupScreen::MultiDeviceSetupOOBEUserChoice>(
-        "MultiDeviceSetup.OOBE.UserChoice",
-        MultiDeviceSetupScreen::MultiDeviceSetupOOBEUserChoice::kAccepted, 1);
-    histogram_tester_.ExpectBucketCount<
-        MultiDeviceSetupScreen::MultiDeviceSetupOOBEUserChoice>(
-        "MultiDeviceSetup.OOBE.UserChoice",
-        MultiDeviceSetupScreen::MultiDeviceSetupOOBEUserChoice::kDeclined, 0);
-
-    multi_device_setup_screen_->OnUserAction("setup-declined");
-
-    histogram_tester_.ExpectBucketCount<
-        MultiDeviceSetupScreen::MultiDeviceSetupOOBEUserChoice>(
-        "MultiDeviceSetup.OOBE.UserChoice",
-        MultiDeviceSetupScreen::MultiDeviceSetupOOBEUserChoice::kAccepted, 1);
-    histogram_tester_.ExpectBucketCount<
-        MultiDeviceSetupScreen::MultiDeviceSetupOOBEUserChoice>(
-        "MultiDeviceSetup.OOBE.UserChoice",
-        MultiDeviceSetupScreen::MultiDeviceSetupOOBEUserChoice::kDeclined, 1);
-  }
-
- private:
-  base::HistogramTester histogram_tester_;
-
-  // Accessory objects needed by MultiDeviceSetupScreen
-  FakeMultiDeviceSetupScreenView fake_multi_device_setup_screen_view_;
-
-  DISALLOW_COPY_AND_ASSIGN(MultiDeviceSetupScreenTest);
-};
-
-TEST_F(MultiDeviceSetupScreenTest, VerifyUserChoicePaths) {
-  VerifyUserChoicePaths();
-}
-
-}  // namespace chromeos

chrome/browser/chromeos/login/wizard_controller.cc

@@ -1256,8 +1256,10 @@ void WizardController::OnAssistantOptInFlowScreenExit(
   ShowMultiDeviceSetupScreen();
 }
 
-void WizardController::OnMultiDeviceSetupScreenExit() {
-  OnScreenExit(MultiDeviceSetupScreenView::kScreenId, kDefaultExitReason);
+void WizardController::OnMultiDeviceSetupScreenExit(
+    MultiDeviceSetupScreen::Result result) {
+  OnScreenExit(MultiDeviceSetupScreenView::kScreenId,
+               MultiDeviceSetupScreen::GetResultString(result));
 
   ShowGestureNavigationScreen();
 }

chrome/browser/chromeos/login/wizard_controller.h

@@ -36,6 +36,7 @@
 #include "chrome/browser/chromeos/login/screens/hid_detection_screen.h"
 #include "chrome/browser/chromeos/login/screens/kiosk_autolaunch_screen.h"
 #include "chrome/browser/chromeos/login/screens/marketing_opt_in_screen.h"
+#include "chrome/browser/chromeos/login/screens/multidevice_setup_screen.h"
 #include "chrome/browser/chromeos/login/screens/network_screen.h"
 #include "chrome/browser/chromeos/login/screens/packaged_license_screen.h"
 #include "chrome/browser/chromeos/login/screens/recommend_apps_screen.h"
@@ -267,7 +268,7 @@ class WizardController {
   void OnRecommendAppsScreenExit(RecommendAppsScreen::Result result);
   void OnAppDownloadingScreenExit();
   void OnAssistantOptInFlowScreenExit(AssistantOptInFlowScreen::Result result);
-  void OnMultiDeviceSetupScreenExit();
+  void OnMultiDeviceSetupScreenExit(MultiDeviceSetupScreen::Result result);
   void OnGestureNavigationScreenExit(GestureNavigationScreen::Result result);
   void OnMarketingOptInScreenExit(MarketingOptInScreen::Result result);
   void OnResetScreenExit();

chrome/test/BUILD.gn

@@ -2327,6 +2327,7 @@ if (!is_android) {
         "../browser/chromeos/login/screens/mock_eula_screen.h",
         "../browser/chromeos/login/screens/mock_wrong_hwid_screen.cc",
         "../browser/chromeos/login/screens/mock_wrong_hwid_screen.h",
+        "../browser/chromeos/login/screens/multidevice_setup_screen_browsertest.cc",
         "../browser/chromeos/login/screens/network_screen_browsertest.cc",
         "../browser/chromeos/login/screens/packaged_license_screen_browsertest.cc",
         "../browser/chromeos/login/screens/recommend_apps/scoped_test_recommend_apps_fetcher_factory.cc",