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

chrome/browser/chromeos/arc/arc_session_manager_unittest.cc

Index: chrome/browser/chromeos/arc/arc_session_manager_unittest.cc
diff --git a/chrome/browser/chromeos/arc/arc_session_manager_unittest.cc b/chrome/browser/chromeos/arc/arc_session_manager_unittest.cc
index 1af0884c20166088c6023d69340d1c859b226573..0505366d250d40ac882e8e964b4bd6f5b7668a85 100644
--- a/chrome/browser/chromeos/arc/arc_session_manager_unittest.cc
+++ b/chrome/browser/chromeos/arc/arc_session_manager_unittest.cc
@@ -13,13 +13,21 @@
 #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"
@@ -48,7 +56,27 @@
 
 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),
@@ -96,7 +124,7 @@ class ArcSessionManagerTestBase : public testing::Test {
   }
 
  protected:
-  Profile* profile() { return profile_.get(); }
+  TestingProfile* profile() { return profile_.get(); }
 
   ArcSessionManager* arc_session_manager() {
     return arc_session_manager_.get();
@@ -526,4 +554,119 @@ TEST_F(ArcSessionManagerKioskTest, AuthFailure) {
   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