Revert "MultiDeviceSetupScreen: Skip screen properly."

This reverts commit 6549ee05.

Reason for revert: Likely culprit for crash/timeout on:

MultiDeviceSetupScreenTest.Accepted
MultiDeviceSetupScreenTest.Declined
MultiDeviceSetupScreenTest.Skipped

https://ci.chromium.org/p/chrome/builders/ci/linux-chromeos-chrome/7383

Original change's description:
> 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/+/2166263
> Reviewed-by: Roman Sorokin [CET] <rsorokin@chromium.org>
> Commit-Queue: Roman Aleksandrov <raleksandrov@google.com>
> Cr-Commit-Position: refs/heads/master@{#793103}

TBR=rsorokin@chromium.org,raleksandrov@google.com

Change-Id: Ic190563d2e266731d1fab4684b23adae5f487163
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1064561
Bug: 1095062
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2329424Reviewed-by: Balazs Engedy <engedy@chromium.org>
Commit-Queue: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#793124}

chrome/browser/chromeos/BUILD.gn

@@ -3205,6 +3205,7 @@ 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,6 +724,7 @@ 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,19 +24,9 @@ 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 ScreenExitCallback& exit_callback)
+    const base::RepeatingClosure& exit_callback)
     : BaseScreen(MultiDeviceSetupScreenView::kScreenId,
                  OobeScreenPriority::DEFAULT),
       view_(view),
@@ -49,40 +39,32 @@ MultiDeviceSetupScreen::~MultiDeviceSetupScreen() {
   view_->Bind(nullptr);
 }
 
-void MultiDeviceSetupScreen::TryInitSetupClient() {
-  if (!setup_client_) {
-    setup_client_ =
-        multidevice_setup::MultiDeviceSetupClientFactory::GetForProfile(
-            ProfileManager::GetActiveUserProfile());
-  }
-}
-
-bool MultiDeviceSetupScreen::MaybeSkip(WizardContext* /*context*/) {
+void MultiDeviceSetupScreen::ShowImpl() {
   // Only attempt the setup flow for non-guest users.
   if (chrome_user_manager_util::IsPublicSessionOrEphemeralLogin()) {
-    exit_callback_.Run(Result::NOT_APPLICABLE);
-    return true;
+    ExitScreen();
+    return;
+  }
+
+  multidevice_setup::MultiDeviceSetupClient* client =
+      multidevice_setup::MultiDeviceSetupClientFactory::GetForProfile(
+          ProfileManager::GetActiveUserProfile());
+
+  if (!client) {
+    ExitScreen();
+    return;
   }
 
-  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 (!setup_client_) {
-    exit_callback_.Run(Result::NOT_APPLICABLE);
-    return true;
-  }
-  if (setup_client_->GetHostStatus().first !=
+  if (client->GetHostStatus().first !=
       multidevice_setup::mojom::HostStatus::kEligibleHostExistsButNoHostSet) {
     VLOG(1) << "Skipping MultiDevice setup screen; host status: "
-            << setup_client_->GetHostStatus().first;
-    exit_callback_.Run(Result::NOT_APPLICABLE);
-    return true;
+            << client->GetHostStatus().first;
+    ExitScreen();
+    return;
   }
 
-  return false;
-}
-
-void MultiDeviceSetupScreen::ShowImpl() {
   view_->Show();
 
   // Record that user was presented with setup flow to prevent spam
@@ -102,11 +84,11 @@ void MultiDeviceSetupScreen::OnUserAction(const std::string& action_id) {
   if (action_id == kAcceptedSetupUserAction) {
     RecordMultiDeviceSetupOOBEUserChoiceHistogram(
         MultiDeviceSetupOOBEUserChoice::kAccepted);
-    exit_callback_.Run(Result::NEXT);
+    ExitScreen();
   } else if (action_id == kDeclinedSetupUserAction) {
     RecordMultiDeviceSetupOOBEUserChoiceHistogram(
         MultiDeviceSetupOOBEUserChoice::kDeclined);
-    exit_callback_.Run(Result::NEXT);
+    ExitScreen();
   } else {
     BaseScreen::OnUserAction(action_id);
     NOTREACHED();
@@ -118,4 +100,8 @@ 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,50 +7,22 @@
 
 #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 ScreenExitCallback& exit_callback);
+                         const base::RepeatingClosure& 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;
@@ -68,16 +40,14 @@ class MultiDeviceSetupScreen : public BaseScreen {
     kMaxValue = kDeclined
   };
 
-  // Inits |setup_client_| if it was not initialized before.
-  void TryInitSetupClient();
-
   static void RecordMultiDeviceSetupOOBEUserChoiceHistogram(
       MultiDeviceSetupOOBEUserChoice value);
 
-  multidevice_setup::MultiDeviceSetupClient* setup_client_ = nullptr;
+  // Exits the screen.
+  void ExitScreen();
 
   MultiDeviceSetupScreenView* view_;
-  ScreenExitCallback exit_callback_;
+  base::RepeatingClosure 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,10 +1256,8 @@ void WizardController::OnAssistantOptInFlowScreenExit(
   ShowMultiDeviceSetupScreen();
 }
 
-void WizardController::OnMultiDeviceSetupScreenExit(
-    MultiDeviceSetupScreen::Result result) {
-  OnScreenExit(MultiDeviceSetupScreenView::kScreenId,
-               MultiDeviceSetupScreen::GetResultString(result));
+void WizardController::OnMultiDeviceSetupScreenExit() {
+  OnScreenExit(MultiDeviceSetupScreenView::kScreenId, kDefaultExitReason);
 
   ShowGestureNavigationScreen();
 }

chrome/browser/chromeos/login/wizard_controller.h

@@ -36,7 +36,6 @@
 #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"
@@ -268,7 +267,7 @@ class WizardController {
   void OnRecommendAppsScreenExit(RecommendAppsScreen::Result result);
   void OnAppDownloadingScreenExit();
   void OnAssistantOptInFlowScreenExit(AssistantOptInFlowScreen::Result result);
-  void OnMultiDeviceSetupScreenExit(MultiDeviceSetupScreen::Result result);
+  void OnMultiDeviceSetupScreenExit();
   void OnGestureNavigationScreenExit(GestureNavigationScreen::Result result);
   void OnMarketingOptInScreenExit(MarketingOptInScreen::Result result);
   void OnResetScreenExit();

chrome/test/BUILD.gn

@@ -2327,7 +2327,6 @@ 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",