Revert of arc: Use the new InstanceHolder for unittests (patchset chr…

…omium#4 id:60001 of https://codereview.chromium.org/2138513002/ )

Reason for revert:
Caused unit_tests failure on Linux ChromiumOS Tests (1):

ArcAppModelBuilderTest.RemoveAppCleanUpFolder

Original issue's description:
> arc: Use the new InstanceHolder for unittests
>
> Instead of having unit tests go through Mojo and jump through hoops to
> ensure that all messages have propagated to all the threads, we can use
> the new arc::ArcBridgeService::InstanceHolder<T>::SetInstance() to
> directly set the concrete fake (and optional version). This simplifies
> testing quite a bit.
>
> BUG=626695
> TEST=trybots
>
> Committed: https://crrev.com/c5fffc64c14a0464f8870bbe91df7e945f42c3ca
> Cr-Commit-Position: refs/heads/master@{#405138}

TBR=hidehiko@chromium.org,stevenjb@chromium.org,lhchavez@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=626695

Review-Url: https://codereview.chromium.org/2146573005
Cr-Commit-Position: refs/heads/master@{#405165}

chrome/browser/ui/app_list/app_context_menu_unittest.cc

@@ -177,10 +177,10 @@ class AppContextMenuTest : public AppListTestBase {
     return profile_.get();
   }
 
-  void AddSeparator(std::vector<MenuState>* states) {
-    if (states->empty() || states->back().command_id == -1)
+  void AddSeparator(std::vector<MenuState>& states) {
+    if (states.empty() || states.back().command_id == -1)
       return;
-    states->push_back(MenuState());
+    states.push_back(MenuState());
   }
 
   void TestExtensionApp(const std::string& app_id,
@@ -206,15 +206,15 @@ class AppContextMenuTest : public AppListTestBase {
     if (!platform_app)
       states.push_back(MenuState(app_list::AppContextMenu::LAUNCH_NEW));
     if (pinnable != AppListControllerDelegate::NO_PIN) {
-      AddSeparator(&states);
+      AddSeparator(states);
       states.push_back(MenuState(
           app_list::AppContextMenu::TOGGLE_PIN,
           pinnable != AppListControllerDelegate::PIN_FIXED,
           false));
     }
     if (can_create_shortcuts)
       states.push_back(MenuState(app_list::AppContextMenu::CREATE_SHORTCUTS));
-    AddSeparator(&states);
+    AddSeparator(states);
 
     if (!platform_app) {
       if (extensions::util::CanHostedAppsOpenInWindows() &&
@@ -244,7 +244,7 @@ class AppContextMenuTest : public AppListTestBase {
             true,
             launch_type == extensions::LAUNCH_TYPE_FULLSCREEN));
       }
-      AddSeparator(&states);
+      AddSeparator(states);
       states.push_back(MenuState(app_list::AppContextMenu::OPTIONS,
                                  false,
                                  false));
@@ -376,6 +376,7 @@ TEST_F(AppContextMenuTest, ArcMenu) {
   EXPECT_EQ(0u, arc_test.app_instance()->launch_requests().size());
 
   menu->ActivatedAt(0);
+  arc_test.app_instance()->WaitForIncomingMethodCall();
 
   const ScopedVector<arc::FakeAppInstance::Request>& launch_requests =
       arc_test.app_instance()->launch_requests();

chrome/browser/ui/app_list/arc/arc_app_test.cc

@@ -110,7 +110,10 @@ void ArcAppTest::SetUp(Profile* profile) {
   DCHECK(arc_app_list_pref_);
 
   app_instance_.reset(new arc::FakeAppInstance(arc_app_list_pref_));
-  bridge_service_->app()->SetInstance(app_instance_.get());
+  arc::mojom::AppInstancePtr instance;
+  app_instance_->Bind(mojo::GetProxy(&instance));
+  bridge_service_->OnAppInstanceReady(std::move(instance));
+  app_instance_->WaitForOnAppInstanceReady();
 
   // Check initial conditions.
   EXPECT_EQ(bridge_service_.get(), arc::ArcBridgeService::Get());

chrome/browser/ui/app_list/arc/arc_app_unittest.cc

@@ -538,8 +538,11 @@ TEST_F(ArcAppModelBuilderTest, LaunchApps) {
   ASSERT_NE(nullptr, item_first);
   ASSERT_NE(nullptr, item_last);
   item_first->Activate(0);
+  app_instance()->WaitForIncomingMethodCall();
   item_last->Activate(0);
+  app_instance()->WaitForIncomingMethodCall();
   item_first->Activate(0);
+  app_instance()->WaitForIncomingMethodCall();
 
   const ScopedVector<arc::FakeAppInstance::Request>& launch_requests =
       app_instance()->launch_requests();
@@ -575,8 +578,11 @@ TEST_F(ArcAppModelBuilderTest, LaunchShortcuts) {
   ASSERT_NE(nullptr, item_first);
   ASSERT_NE(nullptr, item_last);
   item_first->Activate(0);
+  app_instance()->WaitForIncomingMethodCall();
   item_last->Activate(0);
+  app_instance()->WaitForIncomingMethodCall();
   item_first->Activate(0);
+  app_instance()->WaitForIncomingMethodCall();
 
   const ScopedVector<mojo::String>& launch_intents =
       app_instance()->launch_intents();
@@ -622,7 +628,14 @@ TEST_F(ArcAppModelBuilderTest, RequestIcons) {
   // Process pending tasks.
   content::BrowserThread::GetBlockingPool()->FlushForTesting();
   base::RunLoop().RunUntilIdle();
+  // Normally just one call to RunUntilIdle() suffices to make sure
+  // all RequestAppIcon() calls are delivered, but on slower machines
+  // (especially when running under Valgrind), they might not get
+  // delivered on time. Wait for the remaining tasks individually.
   const size_t expected_size = scale_factors.size() * fake_apps().size();
+  while (app_instance()->icon_requests().size() < expected_size) {
+    app_instance()->WaitForIncomingMethodCall();
+  }
 
   // At this moment we should receive all requests for icon loading.
   const ScopedVector<arc::FakeAppInstance::IconRequest>& icon_requests =
@@ -922,6 +935,7 @@ TEST_F(ArcAppModelBuilderTest, AppLauncher) {
   app_instance()->SendRefreshAppList(apps);
 
   EXPECT_TRUE(launcher1.app_launched());
+  app_instance()->WaitForIncomingMethodCall();
   ASSERT_EQ(1u, app_instance()->launch_requests().size());
   EXPECT_TRUE(app_instance()->launch_requests()[0]->IsForApp(app1));
   EXPECT_FALSE(launcher3.app_launched());
@@ -930,6 +944,7 @@ TEST_F(ArcAppModelBuilderTest, AppLauncher) {
 
   ArcAppLauncher launcher2(profile(), id2, true);
   EXPECT_TRUE(launcher2.app_launched());
+  app_instance()->WaitForIncomingMethodCall();
   EXPECT_FALSE(prefs->HasObserver(&launcher2));
   ASSERT_EQ(2u, app_instance()->launch_requests().size());
   EXPECT_TRUE(app_instance()->launch_requests()[1]->IsForApp(app2));

components/arc/arc_bridge_service.cc

@@ -52,6 +52,113 @@ void ArcBridgeService::RemoveObserver(Observer* observer) {
   observer_list_.RemoveObserver(observer);
 }
 
+void ArcBridgeService::OnAppInstanceReady(mojom::AppInstancePtr app_ptr) {
+  DCHECK(CalledOnValidThread());
+  app_.OnInstanceReady(std::move(app_ptr));
+}
+
+void ArcBridgeService::OnAudioInstanceReady(mojom::AudioInstancePtr audio_ptr) {
+  DCHECK(CalledOnValidThread());
+  audio_.OnInstanceReady(std::move(audio_ptr));
+}
+
+void ArcBridgeService::OnAuthInstanceReady(mojom::AuthInstancePtr auth_ptr) {
+  DCHECK(CalledOnValidThread());
+  auth_.OnInstanceReady(std::move(auth_ptr));
+}
+
+void ArcBridgeService::OnBluetoothInstanceReady(
+    mojom::BluetoothInstancePtr bluetooth_ptr) {
+  DCHECK(CalledOnValidThread());
+  bluetooth_.OnInstanceReady(std::move(bluetooth_ptr));
+}
+
+void ArcBridgeService::OnClipboardInstanceReady(
+    mojom::ClipboardInstancePtr clipboard_ptr) {
+  DCHECK(CalledOnValidThread());
+  clipboard_.OnInstanceReady(std::move(clipboard_ptr));
+}
+
+void ArcBridgeService::OnCrashCollectorInstanceReady(
+    mojom::CrashCollectorInstancePtr crash_collector_ptr) {
+  DCHECK(CalledOnValidThread());
+  crash_collector_.OnInstanceReady(std::move(crash_collector_ptr));
+}
+
+void ArcBridgeService::OnFileSystemInstanceReady(
+    mojom::FileSystemInstancePtr file_system_ptr) {
+  DCHECK(CalledOnValidThread());
+  file_system_.OnInstanceReady(std::move(file_system_ptr));
+}
+
+void ArcBridgeService::OnImeInstanceReady(mojom::ImeInstancePtr ime_ptr) {
+  DCHECK(CalledOnValidThread());
+  ime_.OnInstanceReady(std::move(ime_ptr));
+}
+
+void ArcBridgeService::OnIntentHelperInstanceReady(
+    mojom::IntentHelperInstancePtr intent_helper_ptr) {
+  DCHECK(CalledOnValidThread());
+  intent_helper_.OnInstanceReady(std::move(intent_helper_ptr));
+}
+
+void ArcBridgeService::OnMetricsInstanceReady(
+    mojom::MetricsInstancePtr metrics_ptr) {
+  DCHECK(CalledOnValidThread());
+  metrics_.OnInstanceReady(std::move(metrics_ptr));
+}
+
+void ArcBridgeService::OnNetInstanceReady(mojom::NetInstancePtr net_ptr) {
+  DCHECK(CalledOnValidThread());
+  net_.OnInstanceReady(std::move(net_ptr));
+}
+
+void ArcBridgeService::OnNotificationsInstanceReady(
+    mojom::NotificationsInstancePtr notifications_ptr) {
+  DCHECK(CalledOnValidThread());
+  notifications_.OnInstanceReady(std::move(notifications_ptr));
+}
+
+void ArcBridgeService::OnObbMounterInstanceReady(
+    mojom::ObbMounterInstancePtr obb_mounter_ptr) {
+  DCHECK(CalledOnValidThread());
+  obb_mounter_.OnInstanceReady(std::move(obb_mounter_ptr));
+}
+
+void ArcBridgeService::OnPolicyInstanceReady(
+    mojom::PolicyInstancePtr policy_ptr) {
+  DCHECK(CalledOnValidThread());
+  policy_.OnInstanceReady(std::move(policy_ptr));
+}
+
+void ArcBridgeService::OnPowerInstanceReady(mojom::PowerInstancePtr power_ptr) {
+  DCHECK(CalledOnValidThread());
+  power_.OnInstanceReady(std::move(power_ptr));
+}
+
+void ArcBridgeService::OnProcessInstanceReady(
+    mojom::ProcessInstancePtr process_ptr) {
+  DCHECK(CalledOnValidThread());
+  process_.OnInstanceReady(std::move(process_ptr));
+}
+
+void ArcBridgeService::OnStorageManagerInstanceReady(
+    mojom::StorageManagerInstancePtr storage_manager_ptr) {
+  DCHECK(CalledOnValidThread());
+  storage_manager_.OnInstanceReady(std::move(storage_manager_ptr));
+}
+
+void ArcBridgeService::OnVideoInstanceReady(mojom::VideoInstancePtr video_ptr) {
+  DCHECK(CalledOnValidThread());
+  video_.OnInstanceReady(std::move(video_ptr));
+}
+
+void ArcBridgeService::OnWindowManagerInstanceReady(
+    mojom::WindowManagerInstancePtr window_manager_ptr) {
+  DCHECK(CalledOnValidThread());
+  window_manager_.OnInstanceReady(std::move(window_manager_ptr));
+}
+
 void ArcBridgeService::SetState(State state) {
   DCHECK(CalledOnValidThread());
   DCHECK_NE(state_, state);
@@ -74,4 +181,28 @@ bool ArcBridgeService::CalledOnValidThread() {
   return thread_checker_.CalledOnValidThread();
 }
 
+void ArcBridgeService::CloseAllChannels() {
+  // Call all the error handlers of all the channels to both close the channel
+  // and notify any observers that the channel is closed.
+  app_.CloseChannel();
+  audio_.CloseChannel();
+  auth_.CloseChannel();
+  bluetooth_.CloseChannel();
+  clipboard_.CloseChannel();
+  crash_collector_.CloseChannel();
+  file_system_.CloseChannel();
+  ime_.CloseChannel();
+  intent_helper_.CloseChannel();
+  metrics_.CloseChannel();
+  net_.CloseChannel();
+  notifications_.CloseChannel();
+  obb_mounter_.CloseChannel();
+  policy_.CloseChannel();
+  power_.CloseChannel();
+  process_.CloseChannel();
+  storage_manager_.CloseChannel();
+  video_.CloseChannel();
+  window_manager_.CloseChannel();
+}
+
 }  // namespace arc

components/arc/arc_bridge_service.h

@@ -28,7 +28,7 @@ class ArcBridgeTest;
 // The Chrome-side service that handles ARC instances and ARC bridge creation.
 // This service handles the lifetime of ARC instances and sets up the
 // communication channel (the ARC bridge) used to send and receive messages.
-class ArcBridgeService {
+class ArcBridgeService : public mojom::ArcBridgeHost {
  public:
   // Notifies life cycle events of ArcBridgeService.
   class Observer {
@@ -44,7 +44,7 @@ class ArcBridgeService {
     virtual ~Observer() {}
   };
 
-  virtual ~ArcBridgeService();
+  ~ArcBridgeService() override;
 
   // Gets the global instance of the ARC Bridge Service. This can only be
   // called on the thread that this class was created on.
@@ -108,6 +108,36 @@ class ArcBridgeService {
     return &window_manager_;
   }
 
+  // ArcHost:
+  void OnAppInstanceReady(mojom::AppInstancePtr app_ptr) override;
+  void OnAudioInstanceReady(mojom::AudioInstancePtr audio_ptr) override;
+  void OnAuthInstanceReady(mojom::AuthInstancePtr auth_ptr) override;
+  void OnBluetoothInstanceReady(
+      mojom::BluetoothInstancePtr bluetooth_ptr) override;
+  void OnClipboardInstanceReady(
+      mojom::ClipboardInstancePtr clipboard_ptr) override;
+  void OnCrashCollectorInstanceReady(
+      mojom::CrashCollectorInstancePtr crash_collector_ptr) override;
+  void OnFileSystemInstanceReady(
+      mojom::FileSystemInstancePtr file_system_ptr) override;
+  void OnImeInstanceReady(mojom::ImeInstancePtr ime_ptr) override;
+  void OnIntentHelperInstanceReady(
+      mojom::IntentHelperInstancePtr intent_helper_ptr) override;
+  void OnMetricsInstanceReady(mojom::MetricsInstancePtr metrics_ptr) override;
+  void OnNetInstanceReady(mojom::NetInstancePtr net_ptr) override;
+  void OnNotificationsInstanceReady(
+      mojom::NotificationsInstancePtr notifications_ptr) override;
+  void OnObbMounterInstanceReady(
+      mojom::ObbMounterInstancePtr obb_mounter_ptr) override;
+  void OnPolicyInstanceReady(mojom::PolicyInstancePtr policy_ptr) override;
+  void OnPowerInstanceReady(mojom::PowerInstancePtr power_ptr) override;
+  void OnProcessInstanceReady(mojom::ProcessInstancePtr process_ptr) override;
+  void OnStorageManagerInstanceReady(
+      mojom::StorageManagerInstancePtr storage_manager_ptr) override;
+  void OnVideoInstanceReady(mojom::VideoInstancePtr video_ptr) override;
+  void OnWindowManagerInstanceReady(
+      mojom::WindowManagerInstancePtr window_manager_ptr) override;
+
   // Gets if ARC is available in this system.
   bool available() const { return available_; }
 
@@ -156,6 +186,29 @@ class ArcBridgeService {
 
   ArcBridgeService();
 
+  // Gets the current state of the bridge service.
+  State state() const { return state_; }
+
+  // Changes the current state and notifies all observers.
+  void SetState(State state);
+
+  // Changes the current availability and notifies all observers.
+  void SetAvailable(bool availability);
+
+  base::ObserverList<Observer>& observer_list() { return observer_list_; }
+
+  bool CalledOnValidThread();
+
+  // Closes all Mojo channels.
+  void CloseAllChannels();
+
+ private:
+  friend class ArcBridgeTest;
+  FRIEND_TEST_ALL_PREFIXES(ArcBridgeTest, Basic);
+  FRIEND_TEST_ALL_PREFIXES(ArcBridgeTest, Prerequisites);
+  FRIEND_TEST_ALL_PREFIXES(ArcBridgeTest, ShutdownMidStartup);
+  FRIEND_TEST_ALL_PREFIXES(ArcBridgeTest, Restart);
+
   // Instance holders.
   InstanceHolder<mojom::AppInstance> app_;
   InstanceHolder<mojom::AudioInstance> audio_;
@@ -177,26 +230,6 @@ class ArcBridgeService {
   InstanceHolder<mojom::VideoInstance> video_;
   InstanceHolder<mojom::WindowManagerInstance> window_manager_;
 
-  // Gets the current state of the bridge service.
-  State state() const { return state_; }
-
-  // Changes the current state and notifies all observers.
-  void SetState(State state);
-
-  // Changes the current availability and notifies all observers.
-  void SetAvailable(bool availability);
-
-  base::ObserverList<Observer>& observer_list() { return observer_list_; }
-
-  bool CalledOnValidThread();
-
- private:
-  friend class ArcBridgeTest;
-  FRIEND_TEST_ALL_PREFIXES(ArcBridgeTest, Basic);
-  FRIEND_TEST_ALL_PREFIXES(ArcBridgeTest, Prerequisites);
-  FRIEND_TEST_ALL_PREFIXES(ArcBridgeTest, ShutdownMidStartup);
-  FRIEND_TEST_ALL_PREFIXES(ArcBridgeTest, Restart);
-
   base::ObserverList<Observer> observer_list_;
 
   base::ThreadChecker thread_checker_;