arc: ARC loading progress should not be shown when started from OOBE.

chrome/browser/chromeos/arc/arc_session_manager_unittest.cc

 // Copyright 2016 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 <memory>
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
+#include "base/observer_list.h"
 #include "base/run_loop.h"
 #include "chrome/browser/chromeos/arc/arc_optin_uma.h"
 #include "chrome/browser/chromeos/arc/arc_session_manager.h"
+#include "chrome/browser/chromeos/arc/arc_session_manager_data_removed_waiter.h"
+#include "chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h"
+#include "chrome/browser/chromeos/login/screens/arc_terms_of_service_screen_actor.h"
+#include "chrome/browser/chromeos/login/screens/arc_terms_of_service_screen_actor_observer.h"
+#include "chrome/browser/chromeos/login/ui/mock_login_display_host.h"
 #include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
 #include "chrome/browser/chromeos/login/users/scoped_user_manager_enabler.h"
 #include "chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chrome/browser/policy/profile_policy_connector.h"
+#include "chrome/browser/policy/profile_policy_connector_factory.h"
 #include "chrome/browser/prefs/pref_service_syncable_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_list_prefs.h"
 #include "chrome/browser/ui/ash/multi_user/multi_user_util.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/testing_profile.h"
 #include "chromeos/chromeos_switches.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/fake_session_manager_client.h"
 #include "components/arc/arc_service_manager.h"
 #include "components/arc/arc_session_runner.h"
 #include "components/arc/test/fake_arc_session.h"
 #include "components/prefs/pref_service.h"
 #include "components/signin/core/account_id/account_id.h"
 #include "components/sync/model/fake_sync_change_processor.h"
 #include "components/sync/model/sync_error_factory_mock.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "components/user_manager/user_manager.h"
 #include "components/user_manager/user_names.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "google_apis/gaia/gaia_constants.h"
 #include "google_apis/gaia/gaia_urls.h"
 #include "net/http/http_status_code.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace arc {
 
-class ArcSessionManagerTestBase : public testing::Test {
+namespace {
+
+class FakeLoginDisplayHost : public chromeos::MockLoginDisplayHost {

[hidehiko, 2017/01/30 10:12:35]

In Chrome, AFAIK, "Fake" is used meaning the light-weight real implementation.
Mock is used in slightly more various way, but in many cases, including this
case, gMock instance.
Please get rid of Mock inheritance here.

Please see also my comments below about gMock usage.

[khmel, 2017/01/31 02:47:08]

Done.

+ public:
+  FakeLoginDisplayHost() {
+    DCHECK(!chromeos::LoginDisplayHost::default_host_);
+    chromeos::LoginDisplayHost::default_host_ = this;
+  }
+
+  ~FakeLoginDisplayHost() override {
+    DCHECK_EQ(chromeos::LoginDisplayHost::default_host_, this);
+    chromeos::LoginDisplayHost::default_host_ = nullptr;
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(FakeLoginDisplayHost);
+};
+
+}  // namespace
+
+class ArcSessionManagerTestBase : public testing::TestWithParam<bool> {

[hidehiko, 2017/01/30 10:12:35]

Because the param is not used in this base, please document what it is for.

  public:
   ArcSessionManagerTestBase()
       : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP),
         user_manager_enabler_(new chromeos::FakeChromeUserManager()) {}
   ~ArcSessionManagerTestBase() override = default;
 
   void SetUp() override {
     chromeos::DBusThreadManager::GetSetterForTesting()->SetSessionManagerClient(
         base::MakeUnique<chromeos::FakeSessionManagerClient>());
 
@@ skipping 27 matching lines @@
     arc_service_manager_.reset();
     chromeos::DBusThreadManager::Shutdown();
   }
 
   chromeos::FakeChromeUserManager* GetFakeUserManager() const {
     return static_cast<chromeos::FakeChromeUserManager*>(
         user_manager::UserManager::Get());
   }
 
  protected:
-  Profile* profile() { return profile_.get(); }
+  TestingProfile* profile() { return profile_.get(); }
 
   ArcSessionManager* arc_session_manager() {
     return arc_session_manager_.get();
   }
 
   bool WaitForDataRemoved(ArcSessionManager::State expected_state) {
     if (arc_session_manager()->state() !=
         ArcSessionManager::State::REMOVING_DATA_DIR)
       return false;
 
@@ skipping 409 matching lines @@
   // and not invoked then, including TearDown().
   bool terminated = false;
   arc_session_manager()->SetAttemptUserExitCallbackForTesting(
       base::Bind([](bool* terminated) { *terminated = true; }, &terminated));
 
   arc_session_manager()->OnProvisioningFinished(
       ProvisioningResult::CHROME_SERVER_COMMUNICATION_ERROR);
   EXPECT_TRUE(terminated);
 }
 
+class ArcSessionOobeOptInTest : public ArcSessionManagerTest,
+                                public chromeos::ArcTermsOfServiceScreenActor {
+ public:
+  ArcSessionOobeOptInTest() = default;
+
+  void SetUp() override {
+    ArcSessionManagerTest::SetUp();
+
+    ArcTermsOfServiceOobeNegotiator::SetArcTermsOfServiceScreenActorForTesting(
+        this);
+
+    GetFakeUserManager()->set_current_user_new(true);
+
+    base::CommandLine::ForCurrentProcess()->AppendSwitch(
+        chromeos::switches::kEnableArcOOBEOptIn);
+
+    fake_login_display_host_.reset(new FakeLoginDisplayHost());

[hidehiko, 2017/01/30 10:12:35]

MakeUnique, please.

[khmel, 2017/01/31 02:47:08]

Done.

+
+    if (IsManagedUser()) {
+      policy::ProfilePolicyConnector* const connector =
+          policy::ProfilePolicyConnectorFactory::GetForBrowserContext(
+              profile());
+      connector->OverrideIsManagedForTesting(true);
+
+      profile()->GetTestingPrefService()->SetManagedPref(
+          prefs::kArcEnabled, new base::FundamentalValue(true));
+    }
+  }
+
+  void TearDown() override {
+    ArcTermsOfServiceOobeNegotiator::SetArcTermsOfServiceScreenActorForTesting(
+        nullptr);
+
+    ArcSessionManagerTest::TearDown();
+  }
+
+ protected:
+  bool IsManagedUser() { return GetParam(); }
+
+  void ReportResult(bool accepted) {
+    for (auto& observer : observer_list_) {
+      if (accepted)
+        observer.OnAccept();
+      else
+        observer.OnSkip();
+    }
+    base::RunLoop().RunUntilIdle();
+  }
+
+  void MaybeWaitForDataRemoved() {
+    if (IsManagedUser())

[hidehiko, 2017/01/30 10:12:35]

Could you comment why managed user does not need to wait for data removal?

[khmel, 2017/01/31 02:47:08]

Done.

+      return;
+
+    DCHECK_EQ(ArcSessionManager::State::REMOVING_DATA_DIR,
+              ArcSessionManager::Get()->state());
+    ArcSessionManagerDataRemovedWaiter().Wait();
+  }
+
+  chromeos::ArcTermsOfServiceScreenActor* actor() { return this; }
+
+ private:
+  // ArcTermsOfServiceScreenActor:
+  void AddObserver(
+      chromeos::ArcTermsOfServiceScreenActorObserver* observer) override {
+    observer_list_.AddObserver(observer);
+  }
+
+  void RemoveObserver(
+      chromeos::ArcTermsOfServiceScreenActorObserver* observer) override {
+    observer_list_.RemoveObserver(observer);
+  }
+
+  void Show() override {
+    profile()->GetPrefs()->SetBoolean(prefs::kArcEnabled, true);

[hidehiko, 2017/01/30 10:12:35]

Setting the preference and the Show() method name does not directly linked in
general.
Could you comment what are you emulating?

[khmel, 2017/01/31 02:47:08]

Done.

+  }
+
+  void Hide() override {}
+
+  base::ObserverList<chromeos::ArcTermsOfServiceScreenActorObserver>
+      observer_list_;
+  std::unique_ptr<FakeLoginDisplayHost> fake_login_display_host_;

[hidehiko, 2017/01/30 10:12:35]

This is only for let default_host() return true, right?
Then, using gMock here looks overkill.
Also, I'm much worried about the longer term maintenance.
Using gMock sometimes provides benefit for testing, specifically if the
interface is well fixed.
However, in Chrome, we edit many classes/interfaces as we want, and Mock
sometimes makes such a maintenance difficult practically.
I strongly recommend to avoid gMock unless it is really necessary.

[khmel, 2017/01/31 02:47:08]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(ArcSessionOobeOptInTest);
+};
+
+INSTANTIATE_TEST_CASE_P(ArcSessionOobeOptInTestImpl,
+                        ArcSessionOobeOptInTest,
+                        ::testing::Values(true, false));
+
+TEST_P(ArcSessionOobeOptInTest, OobeTermsAccepted) {
+  arc_session_manager()->OnPrimaryUserProfilePrepared(profile());

[hidehiko, 2017/01/30 10:12:35]

L647 - L649 looks a part of SetUp() and L656 looks a part of TearDown().
Ditto for below case.

[khmel, 2017/01/31 02:47:08]

Done.

+  actor()->Show();
+  MaybeWaitForDataRemoved();
+  EXPECT_EQ(ArcSessionManager::State::SHOWING_TERMS_OF_SERVICE,
+            arc_session_manager()->state());
+  ReportResult(true);
+  EXPECT_EQ(ArcSessionManager::State::ACTIVE, arc_session_manager()->state());
+  EXPECT_TRUE(arc_session_manager()->IsArcEnabled());
+  // Correctly stop service.
+  arc_session_manager()->Shutdown();
+}
+
+TEST_P(ArcSessionOobeOptInTest, OobeTermsRejected) {
+  arc_session_manager()->OnPrimaryUserProfilePrepared(profile());
+  actor()->Show();
+  MaybeWaitForDataRemoved();
+  EXPECT_EQ(ArcSessionManager::State::SHOWING_TERMS_OF_SERVICE,
+            arc_session_manager()->state());
+  ReportResult(false);
+  EXPECT_EQ(ArcSessionManager::State::STOPPED, arc_session_manager()->state());
+  EXPECT_FALSE(!IsManagedUser() && arc_session_manager()->IsArcEnabled());
+  // Correctly stop service.
+  arc_session_manager()->Shutdown();
+}
+
 }  // namespace arc