arc: Make request to remove Android's data folder persistent.

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 574acaa4a2ca934eb0fd0c5d52ed859fea75a613..7dde85088b9d7fcdd76f29be56dd1715fc03c0eb 100644
--- a/chrome/browser/chromeos/arc/arc_session_manager_unittest.cc
+++ b/chrome/browser/chromeos/arc/arc_session_manager_unittest.cc
@@ -27,6 +27,7 @@
 #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_bridge_service.h"
 #include "components/arc/test/fake_arc_bridge_service.h"
 #include "components/prefs/pref_service.h"
@@ -53,6 +54,9 @@ class ArcSessionManagerTest : public testing::Test {
   ~ArcSessionManagerTest() override = default;
 
   void SetUp() override {
+    chromeos::DBusThreadManager::GetSetterForTesting()->SetSessionManagerClient(
+        base::WrapUnique(new chromeos::FakeSessionManagerClient));

[hidehiko, 2016/12/06 05:05:30]

nit: base::MakeUnique<chromeos::FakeSessionManagerClient>()

http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Prefer-...

[khmel, 2016/12/06 18:16:12]

Done.

+
     chromeos::DBusThreadManager::Initialize();
 
     base::CommandLine::ForCurrentProcess()->AppendSwitch(
@@ -130,6 +134,7 @@ TEST_F(ArcSessionManagerTest, PrefChangeTriggersService) {
   ASSERT_EQ(ArcSessionManager::State::STOPPED, arc_session_manager()->state());
 
   pref->SetBoolean(prefs::kArcEnabled, true);
+  base::RunLoop().RunUntilIdle();
   ASSERT_EQ(ArcSessionManager::State::SHOWING_TERMS_OF_SERVICE,
             arc_session_manager()->state());
 
@@ -196,6 +201,7 @@ TEST_F(ArcSessionManagerTest, BaseWorkflow) {
   ASSERT_EQ(ArcSessionManager::State::STOPPED, arc_session_manager()->state());
 
   profile()->GetPrefs()->SetBoolean(prefs::kArcEnabled, true);
+  base::RunLoop().RunUntilIdle();
 
   // Setting profile and pref initiates a code fetching process.
   ASSERT_EQ(ArcSessionManager::State::SHOWING_TERMS_OF_SERVICE,
@@ -236,6 +242,8 @@ TEST_F(ArcSessionManagerTest, CancelFetchingDisablesArc) {
 
   arc_session_manager()->OnPrimaryUserProfilePrepared(profile());
   pref->SetBoolean(prefs::kArcEnabled, true);
+  base::RunLoop().RunUntilIdle();
+
   ASSERT_EQ(ArcSessionManager::State::SHOWING_TERMS_OF_SERVICE,
             arc_session_manager()->state());
 
@@ -252,6 +260,7 @@ TEST_F(ArcSessionManagerTest, CloseUIKeepsArcEnabled) {
 
   arc_session_manager()->OnPrimaryUserProfilePrepared(profile());
   pref->SetBoolean(prefs::kArcEnabled, true);
+  base::RunLoop().RunUntilIdle();
 
   arc_session_manager()->StartArc();
 
@@ -375,4 +384,53 @@ TEST_F(ArcSessionManagerTest, DisabledForNonPrimaryProfile) {
   arc_session_manager()->Shutdown();
 }
 
+TEST_F(ArcSessionManagerTest, RemoveDataFolder) {
+  profile()->GetPrefs()->SetBoolean(prefs::kArcEnabled, false);
+  // Starting session manager with prefs::kArcEnabled off automatically removes
+  // Android's data folder.
+  arc_session_manager()->OnPrimaryUserProfilePrepared(profile());
+  EXPECT_TRUE(
+      profile()->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested));
+  EXPECT_EQ(ArcSessionManager::State::STOPPED, arc_session_manager()->state());
+  // Enable ARC. Data is removed asyncronously. At this moment session manager
+  // still should be stopped and data removal request is active.
+  profile()->GetPrefs()->SetBoolean(prefs::kArcEnabled, true);
+  EXPECT_TRUE(
+      profile()->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested));
+  EXPECT_EQ(ArcSessionManager::State::STOPPED, arc_session_manager()->state());
+  // Wait until data is removed.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(
+      profile()->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested));
+  EXPECT_EQ(ArcSessionManager::State::SHOWING_TERMS_OF_SERVICE,
+            arc_session_manager()->state());
+  arc_session_manager()->StartArc();
+  EXPECT_EQ(ArcSessionManager::State::ACTIVE, arc_session_manager()->state());
+
+  // Now request to remove data and stop session manager.
+  ArcSessionManager::Get()->RemoveArcData();

[hidehiko, 2016/12/02 08:54:55]

s/ArcSessionManager::Get()/arc_session_manager()/

[khmel, 2016/12/06 03:23:57]

Done.

+  ASSERT_TRUE(
+      profile()->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested));
+  EXPECT_EQ(ArcSessionManager::State::ACTIVE, arc_session_manager()->state());
+  ArcSessionManager::Get()->Shutdown();

[hidehiko, 2016/12/02 08:54:55]

Ditto.

[khmel, 2016/12/06 03:23:57]

Done.

+  base::RunLoop().RunUntilIdle();
+  // Request should persist.
+  ASSERT_TRUE(
+      profile()->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested));
+
+  // Emulate next sign-in. Data should be removed first and ARC started after.

[hidehiko, 2016/12/02 08:54:55]

To emulate re-sign-in, shouldn't we re-instantiate the arc_session_manager()?

[khmel, 2016/12/06 03:23:57]

Hmm, probably I did not get you. Second and next
OnPrimaryUserProfilePrepared(profile()) is very common pattern in this file and
used in many tests here.

[hidehiko, 2016/12/06 05:05:30]

Oh, sorry for my poor explanation.
I was wondering if re-creating the instance can help to remove the check only
for testing at arc_session_manager:L236.
If not, I'm fine to leave this as is in this CL.

Note that, TBH, it's better to recreate ArcSessionManager instance (in any test
to emulate re-sign-in) to make emulation more closer to the ARC's real behavior,
but it looks out of focus of this CL, I think. Let's address it later.

[khmel, 2016/12/06 18:16:12]

I did similar what you suggested in some other tests where ArcAppListPrefs also
included into the testing and recreating managers is required. In this tests we 
can test this component more independently so this is not strictly required.
Also this test does not affect check you mentioned above because we implemented
wait cycles to test that state switches correctly. 

[hidehiko, 2016/12/06 18:32:09]

My understanding is, the check in the callback is for testing situation where
the Shutdown() is called, but the instance is not yet deleted and the message
loop is running (like L416).

Because the callback is bound with ArcSessionManager's weak_ptr, if we can
delete the instance after Shutdown always, the if check should not be there, as
callback should never be called then.

Anyway, I'm fine to leave it for now, and let's address it later.

+  arc_session_manager()->OnPrimaryUserProfilePrepared(profile());
+  EXPECT_TRUE(
+      profile()->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested));
+  EXPECT_EQ(ArcSessionManager::State::STOPPED, arc_session_manager()->state());
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(
+      profile()->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested));
+  EXPECT_EQ(ArcSessionManager::State::SHOWING_TERMS_OF_SERVICE,
+            arc_session_manager()->state());
+  arc_session_manager()->StartArc();
+  EXPECT_EQ(ArcSessionManager::State::ACTIVE, arc_session_manager()->state());
+  ArcSessionManager::Get()->Shutdown();

[hidehiko, 2016/12/02 08:54:55]

Ditto.

[khmel, 2016/12/06 03:23:57]

Done.

+}
+
 }  // namespace arc