Remove ArcSessionManager::Observer::OnArcBridgeShutdown.

chrome/browser/chromeos/arc/arc_session_manager_browsertest.cc

Index: chrome/browser/chromeos/arc/arc_session_manager_browsertest.cc
diff --git a/chrome/browser/chromeos/arc/arc_session_manager_browsertest.cc b/chrome/browser/chromeos/arc/arc_session_manager_browsertest.cc
index 7d7c96d5a844267e6c0416a4d264fa68527ee844..31ac54186f887fcbd75f2e649d13d3b43e556e73 100644
--- a/chrome/browser/chromeos/arc/arc_session_manager_browsertest.cc
+++ b/chrome/browser/chromeos/arc/arc_session_manager_browsertest.cc
@@ -5,6 +5,7 @@
 #include <memory>
 #include <string>
 
+#include "base/auto_reset.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/files/scoped_temp_dir.h"
@@ -64,34 +65,33 @@ constexpr char kFakeGaiaId[] = "1234567890";
 
 namespace arc {
 
-// Observer of ARC bridge shutdown.
-class ArcSessionManagerShutdownObserver : public ArcSessionManager::Observer {
+// Waits for the "arc.enabled" preference change.
+class ArcPlayStoreEnabledChangedWaiter : public ArcSessionManager::Observer {
  public:
-  ArcSessionManagerShutdownObserver() {
+  ArcPlayStoreEnabledChangedWaiter() {
     ArcSessionManager::Get()->AddObserver(this);
   }
 
-  ~ArcSessionManagerShutdownObserver() override {
+  ~ArcPlayStoreEnabledChangedWaiter() override {
     ArcSessionManager::Get()->RemoveObserver(this);
   }
 
   void Wait() {
-    run_loop_.reset(new base::RunLoop);
-    run_loop_->Run();
-    run_loop_.reset();
+    base::RunLoop run_loop;
+    base::AutoReset<base::RunLoop*> reset(&run_loop_, &run_loop);

[Luis Héctor Chávez, 2017/02/15 18:44:50]

nice, this looks _much_ cleaner :)

[hidehiko, 2017/02/15 18:51:53]

Thanks :-).

+    run_loop.Run();
   }
 
-  // ArcSessionManager::Observer:
-  void OnArcBridgeShutdown() override {
-    if (!run_loop_)
-      return;
-    run_loop_->Quit();
+ private:
+  // ArcSessionManager::Observer override:
+  void OnArcPlayStoreEnabledChanged(bool enabled) override {
+    if (run_loop_)

[Luis Héctor Chávez, 2017/02/15 18:44:50]

nit:

if (!enabled) {
  DCHECK(run_loop_);
  run_loop_->Quit();
}

[hidehiko, 2017/02/15 18:51:53]

It changes the semantics (from the brief document of this class), I think.
Do you mean we should create ArcPlayStoreDisabledWaiter?

[Luis Héctor Chávez, 2017/02/15 19:09:44]

So previously the semantics for this class was to wait until ARC session was
shut down. Now this listens for the play store state to change in any direction
(since you're not inspecting the value of |enabled|), which is not compatible
with what was there previously. At the very least this should limit to waiting
until the state goes from enabled to disabled. 

Naming the class ArcPlayStoreDisabledWaiter seems like a good direction, and
maybe also changing the class comment to "Waits for the "arc.enabled" preference
to change from enabled to disabled".

[hidehiko, 2017/02/15 19:52:43]

Ok, thank you for detailed explanation.
Done.

+      run_loop_->Quit();
   }
 
- private:
-  std::unique_ptr<base::RunLoop> run_loop_;
+  base::RunLoop* run_loop_ = nullptr;
 
-  DISALLOW_COPY_AND_ASSIGN(ArcSessionManagerShutdownObserver);
+  DISALLOW_COPY_AND_ASSIGN(ArcPlayStoreEnabledChangedWaiter);
 };
 
 class ArcSessionManagerTest : public InProcessBrowserTest {
@@ -227,7 +227,7 @@ IN_PROC_BROWSER_TEST_F(ArcSessionManagerTest, ManagedAndroidAccount) {
   EnableArc();
   token_service()->IssueTokenForAllPendingRequests(kManagedAuthToken,
                                                    base::Time::Max());
-  ArcSessionManagerShutdownObserver().Wait();
+  ArcPlayStoreEnabledChangedWaiter().Wait();
   ASSERT_EQ(ArcSessionManager::State::REMOVING_DATA_DIR,
             ArcSessionManager::Get()->state());
   ArcDataRemovedWaiter().Wait();