[ARC] Decouple ARC data removal from disabling ARC inside of ArcSessionManager

This is a purely refactoring CL which doesn't modify any behaviour.

Currently ArcSessionManager::RequestDisable also calls
ArcSessionManager::RequestArcDataRemoval - there are use cases where we want to
disable ARC and preserve user data, so decouple the two.

This CL is a preparation for disabling of ARC inside of locked fullscreen mode.
More details can be found on the design doc go/disabling-arc-locked-fullscreen

Bug: 888611
Change-Id: Ifd2aad309c0300416aae3124d3f526ec7cf30a44
Reviewed-on: https://chromium-review.googlesource.com/c/1363202Reviewed-by: Yury Khmel <khmel@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Commit-Queue: Ivan Šandrk <isandrk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#635454}

chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.cc

@@ -163,10 +163,14 @@ void ArcPlayStoreEnabledPreferenceHandler::UpdateArcSessionManager() {
         IsArcPlayStoreEnabledPreferenceManagedForProfile(profile_));
   }
 
-  if (ShouldArcAlwaysStart() || IsArcPlayStoreEnabledForProfile(profile_))
+  if (ShouldArcAlwaysStart() || IsArcPlayStoreEnabledForProfile(profile_)) {
     arc_session_manager_->RequestEnable();
-  else
+  } else {
+    const bool enable_requested = arc_session_manager_->enable_requested();
     arc_session_manager_->RequestDisable();
+    if (enable_requested)
+      arc_session_manager_->RequestArcDataRemoval();
+  }
 }
 
 }  // namespace arc

chrome/browser/chromeos/arc/arc_session_manager.cc

@@ -753,6 +753,8 @@ void ArcSessionManager::RequestDisable() {
     return;
   }
 
+  VLOG(1) << "Disabling ARC.";
+
   directly_started_ = false;
   enable_requested_ = false;
   UpdatePersistentUMAState();
@@ -763,14 +765,13 @@ void ArcSessionManager::RequestDisable() {
   // Reset any pending request to re-enable ARC.
   reenable_arc_ = false;
   StopArc();
-  VLOG(1) << "ARC opt-out. Removing user data.";
-  RequestArcDataRemoval();
 }
 
 void ArcSessionManager::RequestArcDataRemoval() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(profile_);
   DCHECK(data_remover_);
+  VLOG(1) << "Removing user ARC data.";
 
   // TODO(hidehiko): DCHECK the previous state. This is called for four cases;
   // 1) Supporting managed user initial disabled case (Please see also

chrome/browser/chromeos/arc/arc_session_manager.h

@@ -188,7 +188,8 @@ class ArcSessionManager : public ArcSessionRunner::Observer,
 
   // Requests to disable ARC session. This stops ARC instance, or quits Terms
   // Of Service negotiation if it is the middle of the process (e.g. closing
-  // UI for manual negotiation if it is shown).
+  // UI for manual negotiation if it is shown). This does not remove user ARC
+  // data.
   // If it is already requested to disable, no-op.
   void RequestDisable();
 

chrome/browser/chromeos/arc/arc_session_manager_unittest.cc

@@ -424,7 +424,10 @@ TEST_F(ArcSessionManagerTest, CancelFetchingDisablesArc) {
   EXPECT_FALSE(IsArcPlayStoreEnabledForProfile(profile()));
 
   // Emulate the preference handling.
+  const bool enable_requested = arc_session_manager()->enable_requested();
   arc_session_manager()->RequestDisable();
+  if (enable_requested)
+    arc_session_manager()->RequestArcDataRemoval();
 
   // Wait until data is removed.
   ASSERT_TRUE(WaitForDataRemoved(ArcSessionManager::State::STOPPED));
@@ -700,7 +703,10 @@ TEST_F(ArcSessionManagerTest, ClearArcTransitionOnShutdown) {
       prefs::kArcSupervisionTransition,
       static_cast<int>(ArcSupervisionTransition::CHILD_TO_REGULAR));
   // Simulate ARC shutdown.
+  const bool enable_requested = arc_session_manager()->enable_requested();
   arc_session_manager()->RequestDisable();
+  if (enable_requested)
+    arc_session_manager()->RequestArcDataRemoval();
   EXPECT_EQ(
       static_cast<int>(ArcSupervisionTransition::NO_TRANSITION),
       profile()->GetPrefs()->GetInteger(prefs::kArcSupervisionTransition));
@@ -889,6 +895,26 @@ TEST_F(ArcSessionManagerTest, DataCleanUpOnNextStart) {
   arc_session_manager()->Shutdown();
 }
 
+TEST_F(ArcSessionManagerTest, RequestDisableDoesNotRemoveData) {
+  // Start ARC.
+  arc_session_manager()->SetProfile(profile());
+  arc_session_manager()->Initialize();
+  arc_session_manager()->RequestEnable();
+  base::RunLoop().RunUntilIdle();
+  ASSERT_EQ(ArcSessionManager::State::NEGOTIATING_TERMS_OF_SERVICE,
+            arc_session_manager()->state());
+
+  // Disable ARC.
+  arc_session_manager()->RequestDisable();
+
+  // Data removal is not requested.
+  EXPECT_FALSE(
+      profile()->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested));
+
+  // Correctly stop service.
+  arc_session_manager()->Shutdown();
+}
+
 class ArcSessionManagerArcAlwaysStartTest : public ArcSessionManagerTest {
  public:
   ArcSessionManagerArcAlwaysStartTest() = default;

chrome/browser/chromeos/arc/arc_util.cc

@@ -386,10 +386,18 @@ bool SetArcPlayStoreEnabledForProfile(Profile* profile, bool enabled) {
     // |arc_session_manager| can be nullptr in unit_tests.
     if (!arc_session_manager)
       return false;
-    if (enabled)
+    if (enabled) {
       arc_session_manager->RequestEnable();
-    else
+    } else {
+      // Before calling RequestDisable here we cache enable_requested because
+      // RequestArcDataRemoval was refactored outside of RequestDisable where
+      // it was called only in case enable_requested was true (RequestDisable
+      // sets enable_requested to false).
+      const bool enable_requested = arc_session_manager->enable_requested();
       arc_session_manager->RequestDisable();
+      if (enable_requested)
+        arc_session_manager->RequestArcDataRemoval();
+    }
     return true;
   }
   profile->GetPrefs()->SetBoolean(prefs::kArcEnabled, enabled);