app_list: Replace mojom::AppListController

Bug: 958134
Change-Id: I3e5651c7469300d14131f916543ba5c82fc3556b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1616969
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Sam McNally <sammc@chromium.org>
Auto-Submit: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Reviewed-by: Alex Newcomer <newcomer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#661142}

ash/app_list/app_list_controller_impl.cc

@@ -157,6 +157,9 @@ AppListControllerImpl::~AppListControllerImpl() {
   // remove this from objects it's observing.
   if (!is_shutdown_)
     Shutdown();
+
+  if (client_)
+    client_->OnAppListControllerDestroyed();
 }
 
 // static
@@ -171,11 +174,6 @@ void AppListControllerImpl::SetClient(app_list::AppListClient* client) {
   client_ = client;
 }
 
-void AppListControllerImpl::BindRequest(
-    mojom::AppListControllerRequest request) {
-  bindings_.AddBinding(this, std::move(request));
-}
-
 app_list::AppListModel* AppListControllerImpl::GetModel() {
   return model_.get();
 }
@@ -589,10 +587,6 @@ ash::mojom::AppListViewState AppListControllerImpl::GetAppListViewState() {
   return model_->state_fullscreen();
 }
 
-void AppListControllerImpl::FlushForTesting() {
-  bindings_.FlushForTesting();
-}
-
 void AppListControllerImpl::OnShellDestroying() {
   // Stop observing at the beginning of ~Shell to avoid unnecessary work during
   // Shell shutdown.

ash/app_list/app_list_controller_impl.h

@@ -36,8 +36,6 @@
 #include "ash/wm/tablet_mode/tablet_mode_observer.h"
 #include "base/observer_list.h"
 #include "components/sync/model/string_ordinal.h"
-#include "mojo/public/cpp/bindings/binding_set.h"
-#include "mojo/public/cpp/bindings/interface_ptr_set.h"
 
 class PrefRegistrySimple;
 
@@ -54,7 +52,6 @@ class AppListControllerObserver;
 // state.
 class ASH_EXPORT AppListControllerImpl
     : public app_list::AppListController,
-      public mojom::AppListController,
       public SessionObserver,
       public app_list::AppListModelObserver,
       public app_list::AppListViewDelegate,
@@ -71,22 +68,15 @@ class ASH_EXPORT AppListControllerImpl
       public HomeLauncherGestureHandlerObserver,
       public HomeScreenDelegate {
  public:
-  using AppListItemMetadataPtr = mojom::AppListItemMetadataPtr;
-  using SearchResultMetadataPtr = mojom::SearchResultMetadataPtr;
   AppListControllerImpl();
   ~AppListControllerImpl() override;
 
   static void RegisterProfilePrefs(PrefRegistrySimple* registry);
 
-  // Binds the mojom::AppListController interface request to this object.
-  void BindRequest(mojom::AppListControllerRequest request);
-
   app_list::AppListPresenterImpl* presenter() { return &presenter_; }
 
   // app_list::AppListController:
   void SetClient(app_list::AppListClient* client) override;
-
-  // mojom::AppListController:
   void AddItem(AppListItemMetadataPtr app_item) override;
   void AddItemToFolder(AppListItemMetadataPtr app_item,
                        const std::string& folder_id) override;
@@ -222,8 +212,6 @@ class ASH_EXPORT AppListControllerImpl
   void NotifyAppListVisibilityChanged(bool visible, int64_t display_id);
   void NotifyAppListTargetVisibilityChanged(bool visible);
 
-  void FlushForTesting();
-
   // ShellObserver:
   void OnShellDestroying() override;
 
@@ -359,9 +347,6 @@ class ASH_EXPORT AppListControllerImpl
   // in destruction.
   app_list::AppListPresenterImpl presenter_;
 
-  // Bindings for the AppListController interface.
-  mojo::BindingSet<mojom::AppListController> bindings_;
-
   // True if the on-screen keyboard is shown.
   bool onscreen_keyboard_shown_ = false;
 

ash/app_list/app_list_unittest.cc

@@ -46,20 +46,12 @@ TEST_F(AppListTest, PressAppListButtonToShowAndDismiss) {
   generator.set_current_screen_location(
       app_list_button->GetBoundsInScreen().CenterPoint());
   generator.ClickLeftButton();
-  // Flush the mojo message from Ash to Chrome to show the app list.
-  controller->FlushForTesting();
   EXPECT_TRUE(presenter->GetTargetVisibility());
-  // Flush the mojo message from Chrome to Ash reporting the visibility change.
-  EXPECT_TRUE(controller->GetTargetVisibility());
   EXPECT_EQ(1u, app_list_container->children().size());
   EXPECT_TRUE(app_list_button->IsShowingAppList());
 
   // Click the button again to dismiss the app list; it will animate to close.
   generator.ClickLeftButton();
-  // Flush the mojo message from Ash to Chrome to hide the app list.
-  controller->FlushForTesting();
-  EXPECT_FALSE(presenter->GetTargetVisibility());
-  // Flush the mojo message from Chrome to Ash reporting the visibility change.
   EXPECT_FALSE(controller->GetTargetVisibility());
   EXPECT_EQ(1u, app_list_container->children().size());
   EXPECT_FALSE(app_list_button->IsShowingAppList());

ash/app_list/test/app_list_test_helper.cc

@@ -35,7 +35,6 @@ AppListTestHelper::~AppListTestHelper() {
 }
 
 void AppListTestHelper::WaitUntilIdle() {
-  app_list_controller_->FlushForTesting();
   base::RunLoop().RunUntilIdle();
 }
 

ash/app_list/test/app_list_test_helper.h

@@ -24,8 +24,7 @@ class AppListTestHelper {
   AppListTestHelper();
   ~AppListTestHelper();
 
-  // Show the app list in |display_id|, and wait until animation and mojo calls
-  // finish.
+  // Show the app list in |display_id|, and wait until animation finishes.
   // Note: we usually don't care about the show source in tests.
   void ShowAndRunLoop(uint64_t display_id);
 
@@ -33,23 +32,22 @@ class AppListTestHelper {
   void Show(uint64_t display_id);
 
   // Show the app list in |display_id| triggered with |show_source|, and wait
-  // until animation and mojo calls finish.
+  // until animation finishes.
   void ShowAndRunLoop(uint64_t display_id,
                       app_list::AppListShowSource show_source);
 
-  // Dismiss the app list, and wait until animation and mojo calls finish.
+  // Dismiss the app list, and wait until animation finishes.
   void DismissAndRunLoop();
 
   // Dismiss the app list.
   void Dismiss();
 
-  // Toggle the app list in |display_id|, and wait until animation and mojo
-  // calls finish.
+  // Toggle the app list in |display_id|, and wait until animation finishes.
   // Note: we usually don't care about the show source in tests.
   void ToggleAndRunLoop(uint64_t display_id);
 
   // Toggle the app list in |display_id| triggered with |show_source|, and wait
-  // until animation and mojo calls finish.
+  // until animation finishes.
   void ToggleAndRunLoop(uint64_t display_id,
                         app_list::AppListShowSource show_source);
 
@@ -60,7 +58,7 @@ class AppListTestHelper {
   // Check the current app list view state.
   void CheckState(ash::mojom::AppListViewState state);
 
-  // Run all pending in message loop and flush all mojo calls.
+  // Run all pending in message loop to wait for animation to finish.
   void WaitUntilIdle();
 
   app_list::AppListView* GetAppListView();

ash/app_list/test/test_app_list_client.h

@@ -20,6 +20,7 @@ class TestAppListClient : public app_list::AppListClient {
   ~TestAppListClient() override;
 
   // app_list::AppListClient:
+  void OnAppListControllerDestroyed() override {}
   void StartSearch(const base::string16& trimmed_query) override {}
   void OpenSearchResult(const std::string& result_id,
                         int event_flags,

ash/mojo_interface_factory.cc

@@ -66,15 +66,6 @@ void BindAccessibilityFocusRingControllerRequestOnMainThread(
       std::move(request));
 }
 
-void BindAppListControllerRequestOnMainThread(
-    mojom::AppListControllerRequest request) {
-  // The AppListController is not available in KioskNext sessions.
-  // TODO(michaelpg): Also disable the Chrome AppList client in KioskNext
-  // sessions.
-  if (Shell::Get()->app_list_controller())
-    Shell::Get()->app_list_controller()->BindRequest(std::move(request));
-}
-
 void BindAssistantAlarmTimerControllerRequestOnMainThread(
     mojom::AssistantAlarmTimerControllerRequest request) {
   Shell::Get()->assistant_controller()->alarm_timer_controller()->BindRequest(
@@ -233,9 +224,6 @@ void RegisterInterfaces(
       base::BindRepeating(
           &BindAccessibilityFocusRingControllerRequestOnMainThread),
       main_thread_task_runner);
-  registry->AddInterface(
-      base::BindRepeating(&BindAppListControllerRequestOnMainThread),
-      main_thread_task_runner);
   if (chromeos::switches::IsAssistantEnabled()) {
     registry->AddInterface(
         base::BindRepeating(

ash/public/cpp/app_list/app_list_client.h

@@ -19,6 +19,8 @@
 
 namespace app_list {
 
+class AppListController;
+
 // TODO(crbug.com/958134): Remove the alias and app_list.mojom.h include.
 using AppListLaunchedFrom = ash::mojom::AppListLaunchedFrom;
 using AppListLaunchType = ash::mojom::AppListLaunchType;
@@ -35,6 +37,9 @@ using AppListLaunchType = ash::mojom::AppListLaunchType;
 //   in Chrome.
 class ASH_PUBLIC_EXPORT AppListClient {
  public:
+  // Invoked when AppListController is destroyed.
+  virtual void OnAppListControllerDestroyed() = 0;
+
   //////////////////////////////////////////////////////////////////////////////
   // Interfaces on searching:
   // Triggers a search query.

ash/public/cpp/app_list/app_list_controller.h

@@ -6,6 +6,10 @@
 #define ASH_PUBLIC_CPP_APP_LIST_APP_LIST_CONTROLLER_H_
 
 #include "ash/public/cpp/ash_public_export.h"
+// TODO(crbug.com/958134): Remove.
+#include "ash/public/interfaces/app_list.mojom.h"
+#include "base/callback_forward.h"
+#include "base/strings/string16.h"
 
 namespace app_list {
 
@@ -21,12 +25,145 @@ class AppListClient;
 //   happen while installing/uninstalling apps and the app list gets toggled.
 class ASH_PUBLIC_EXPORT AppListController {
  public:
+  using AppListItemMetadataPtr = ash::mojom::AppListItemMetadataPtr;
+  using SearchResultMetadataPtr = ash::mojom::SearchResultMetadataPtr;
+
   // Gets the instance.
   static AppListController* Get();
 
   // Sets a client to handle calls from Ash.
   virtual void SetClient(AppListClient* client) = 0;
 
+  // Adds an item to AppListModel.
+  virtual void AddItem(AppListItemMetadataPtr app_item) = 0;
+
+  // Adds an item into a certain folder in AppListModel.
+  virtual void AddItemToFolder(AppListItemMetadataPtr app_item,
+                               const std::string& folder_id) = 0;
+
+  // Removes an item by its id from AppListModel.
+  virtual void RemoveItem(const std::string& id) = 0;
+
+  // Removes an item by its id, and also cleans up if its parent folder has a
+  // single child left.
+  virtual void RemoveUninstalledItem(const std::string& id) = 0;
+
+  // Moves the item with |id| to the folder with |folder_id|.
+  virtual void MoveItemToFolder(const std::string& id,
+                                const std::string& folder_id) = 0;
+
+  // Tells Ash what the current status of AppListModel should be,
+  // e.g. the model is under synchronization or in normal status.
+  virtual void SetStatus(ash::AppListModelStatus status) = 0;
+
+  // Tells Ash what the current state of the app list should be,
+  // e.g. the user is searching for something, or showing apps, etc.
+  virtual void SetState(ash::AppListState state) = 0;
+
+  // Highlights the given item in the app list. If not present and it is later
+  // added, the item will be highlighted after being added.
+  virtual void HighlightItemInstalledFromUI(const std::string& id) = 0;
+
+  // Sets whether the search engine is Google or not.
+  virtual void SetSearchEngineIsGoogle(bool is_google) = 0;
+
+  // Sets the text for screen readers on the search box, and updates the
+  // accessible names.
+  virtual void SetSearchTabletAndClamshellAccessibleName(
+      const base::string16& tablet_accessible_name,
+      const base::string16& clamshell_accessible_name) = 0;
+
+  // Sets the hint text to display when there is in input.
+  virtual void SetSearchHintText(const base::string16& hint_text) = 0;
+
+  // Sets the text for the search box's Textfield and the voice search flag.
+  virtual void UpdateSearchBox(const base::string16& text,
+                               bool initiated_by_user) = 0;
+
+  // Publishes search results to Ash to render them.
+  virtual void PublishSearchResults(
+      std::vector<SearchResultMetadataPtr> results) = 0;
+
+  // Updates an item's metadata (e.g. name, position, etc).
+  virtual void SetItemMetadata(const std::string& id,
+                               AppListItemMetadataPtr data) = 0;
+
+  // Updates an item's icon.
+  virtual void SetItemIcon(const std::string& id,
+                           const gfx::ImageSkia& icon) = 0;
+
+  // Updates whether an item is installing.
+  virtual void SetItemIsInstalling(const std::string& id,
+                                   bool is_installing) = 0;
+
+  // Updates the downloaded percentage of an item.
+  virtual void SetItemPercentDownloaded(const std::string& id,
+                                        int32_t percent_downloaded) = 0;
+
+  // Update the whole model, usually when profile changes happen in Chrome.
+  virtual void SetModelData(int profile_id,
+                            std::vector<AppListItemMetadataPtr> apps,
+                            bool is_search_engine_google) = 0;
+
+  // Updates a search rresult's metadata.
+  virtual void SetSearchResultMetadata(SearchResultMetadataPtr metadata) = 0;
+
+  // Updates whether a search result is being installed.
+  virtual void SetSearchResultIsInstalling(const std::string& id,
+                                           bool is_installing) = 0;
+
+  // Updates the download progress of a search result.
+  virtual void SetSearchResultPercentDownloaded(const std::string& id,
+                                                int32_t percent_downloaded) = 0;
+
+  // Called when the app represented by a search result is installed.
+  virtual void NotifySearchResultItemInstalled(const std::string& id) = 0;
+
+  // Returns a map from each item's id to its shown index in the app list.
+  using GetIdToAppListIndexMapCallback =
+      base::OnceCallback<void(const base::flat_map<std::string, uint16_t>&)>;
+  virtual void GetIdToAppListIndexMap(
+      GetIdToAppListIndexMapCallback callback) = 0;
+
+  // Finds the OEM folder or creates one if it doesn't exist.
+  // |oem_folder_name|: the expected name of the OEM folder while creating.
+  // |preferred_oem_position|: the preferred position of the OEM folder while
+  //                           creating; if it's invalid then the final position
+  //                           is determined in Ash.
+  // |oem_folder|: the meta data of the existing/created OEM folder.
+  using FindOrCreateOemFolderCallback =
+      base::OnceCallback<void(AppListItemMetadataPtr)>;
+  virtual void FindOrCreateOemFolder(
+      const std::string& oem_folder_name,
+      const syncer::StringOrdinal& preferred_oem_position,
+      FindOrCreateOemFolderCallback callback) = 0;
+
+  // Resolves the position of the OEM folder.
+  // |preferred_oem_position|: the preferred position of the OEM folder; if it's
+  //                           invalid then the final position is determined in
+  //                           Ash.
+  // |oem_folder|: the meta data of the OEM folder, or null if it doesn't exist.
+  using ResolveOemFolderPositionCallback =
+      base::OnceCallback<void(AppListItemMetadataPtr)>;
+  virtual void ResolveOemFolderPosition(
+      const syncer::StringOrdinal& preferred_oem_position,
+      ResolveOemFolderPositionCallback callback) = 0;
+
+  // Dismisses the app list.
+  virtual void DismissAppList() = 0;
+
+  // Returns bounds of a rectangle to show an AppInfo dialog.
+  using GetAppInfoDialogBoundsCallback =
+      base::OnceCallback<void(const gfx::Rect&)>;
+  virtual void GetAppInfoDialogBounds(
+      GetAppInfoDialogBoundsCallback callback) = 0;
+
+  // Shows the app list and switches to |state|.
+  virtual void ShowAppListAndSwitchToState(ash::AppListState state) = 0;
+
+  // Shows the app list.
+  virtual void ShowAppList() = 0;
+
  protected:
   AppListController();
   virtual ~AppListController();

ash/public/cpp/manifest.cc

@@ -66,7 +66,7 @@ const service_manager::Manifest& GetManifest() {
               service_manager::Manifest::InterfaceList<
                   mojom::AccessibilityController,
                   mojom::AccessibilityFocusRingController,
-                  mojom::AppListController, mojom::AshMessageCenterController,
+                  mojom::AshMessageCenterController,
                   mojom::AssistantAlarmTimerController,
                   mojom::AssistantController,
                   mojom::AssistantNotificationController,

ash/public/interfaces/app_list.mojom

@@ -163,153 +163,3 @@ struct SearchResultAction {
   gfx.mojom.ImageSkia image;
   bool visible_on_hover;
 };
-
-// The Chrome app list (aka Launcher), is the place where user can find and
-// organize all installed apps, or search for various types of information.
-//
-// For apps:
-// The app list displays apps synced across devices based on a user account, in
-// an order that can be modified by the user. It supports up-to-3-layer app
-// organization, the root app list, folders, and apps:
-// - Each app can stay in the root app list or a folder.
-// - Each folder holds more than one apps, which means it'll automatically get
-//   removed when there's only one app left in it.
-// - The OEM folder is a special folder where we cannot move items to/from it.
-//   And we cannot rename it.
-// - Other folders are renamable.
-// - Folders cannot hold folders.
-// - Different items/folders never have a same GUID.
-// - Every item/folder has a same GUID on different devices.
-//
-// For searching:
-// The app list supports various kinds of searching (e.g. apps, onmibox, etc).
-// And search results can be displayed in different formats (e.g. tiles, cards).
-// - Result ids are url like string, e.g.
-//   "chrome-extension://mgndgikekgjfcpckkfioiadnlibdjbkf/",
-//   "play://hhbckbkcbnemggclionhhgaceohjfdkl", etc.
-// - Different search results never have a same id.
-// - Every search result has a same id on different devices.
-// - Every search result can have a list of actions (e.g. install), see
-//   app_list::SearchResult::Action.
-//
-// Users can long press on any app list item or search result to show a context
-// menu. A context menu has a list of commands (e.g. open, uninstall, etc.).
-// - Different items/results may have different command lists.
-// - Each item/result usually has a same command list, but not always. Consider
-//   when we pin an app to the shelf and when we unpin it, the context menu
-//   looks different.
-// - Inside each command list, each command has its own unique command id.
-// - A same command in different command lists has a unique command id, see
-//   app_list::AppContextMenu::CommandId.
-
-// The Chrome app list has its UI running in Ash, and everything else running in
-// Chrome (e.g. syncing, user profile, etc). This controller is implemented in
-// Ash to handle calls from Chrome. These include:
-// - When app list data changes in Chrome, it should notifies the UI models and
-//   views in Ash to get updated. This can happen while syncing, searching, etc.
-// - When Chrome needs real-time UI information from Ash. This can happen while
-//   calculating recommended search results based on the app list item order.
-// - When app list states in Chrome change that require UI's response. This can
-//   happen while installing/uninstalling apps and the app list gets toggled.
-interface AppListController {
-  //////////////////////////////////////////////////////////////////////////////
-  // Interfaces that come from AppListModelUpdater:
-  // The following interfaces are called to update the app list model in Ash,
-  // including both the app list item model, search result model and search box
-  // model.
-  // Adds an item to AppListModel.
-  AddItem(AppListItemMetadata app_item);
-  // Adds an item into a certain folder in AppListModel.
-  AddItemToFolder(AppListItemMetadata app_item, string folder_id);
-  // Removes an item by its id from AppListModel.
-  RemoveItem(string id);
-  // Removes an item by its id, and also cleans up if its parent folder has a
-  // single child left.
-  RemoveUninstalledItem(string id);
-  // Moves the item with |id| to the folder with |folder_id|.
-  MoveItemToFolder(string id, string folder_id);
-  // Tells Ash what the current status of AppListModel should be,
-  // e.g. the model is under synchronization or in normal status.
-  SetStatus(AppListModelStatus status);
-  // Tells Ash what the current state of the app list should be,
-  // e.g. the user is searching for something, or showing apps, etc.
-  SetState(AppListState state);
-  // Highlights the given item in the app list. If not present and it is later
-  // added, the item will be highlighted after being added.
-  HighlightItemInstalledFromUI(string id);
-  // Sets whether the search engine is Google or not.
-  SetSearchEngineIsGoogle(bool is_google);
-  // Sets the text for screen readers on the search box, and updates the
-  // accessible names.
-  SetSearchTabletAndClamshellAccessibleName(
-      mojo_base.mojom.String16 tablet_accessible_name,
-      mojo_base.mojom.String16 clamshell_accessible_name);
-  // Sets the hint text to display when there is in input.
-  SetSearchHintText(mojo_base.mojom.String16 hint_text);
-  // Sets the text for the search box's Textfield and the voice search flag.
-  UpdateSearchBox(mojo_base.mojom.String16 text, bool initiated_by_user);
-  // Publishes search results to Ash to render them.
-  PublishSearchResults(array<SearchResultMetadata> results);
-  // Update the whole model, usually when profile changes happen in Chrome.
-  SetModelData(int32 profile_id,
-               array<AppListItemMetadata> apps,
-               bool is_search_engine_google);
-
-  //////////////////////////////////////////////////////////////////////////////
-  // Interfaces only used by ChromeAppListItem:
-  // These interfaces are called when an item's data is updated in Chrome.
-  // Updates an item's metadata (e.g. name, position, etc).
-  SetItemMetadata(string id, AppListItemMetadata metadata);
-  // Updates an item's icon.
-  SetItemIcon(string id, gfx.mojom.ImageSkia? icon);
-  // Updates whether an item is installing.
-  SetItemIsInstalling(string id, bool is_installing);
-  // Updates the downloaded percentage of an item.
-  SetItemPercentDownloaded(string id, int32 percent_downloaded);
-
-  //////////////////////////////////////////////////////////////////////////////
-  // Interfaces for item querying:
-  // Returns a map from each item's id to its shown index in the app list.
-  GetIdToAppListIndexMap() => (map<string, uint16> indices);
-
-  //////////////////////////////////////////////////////////////////////////////
-  // Interfaces for AppListSyncableService:
-  // These interfaces are called while dealing with the OEM folder in the
-  // AppListSyncableService in Chrome.
-  // Finds the OEM folder or creates one if it doesn't exist.
-  // |oem_folder_name|: the expected name of the OEM folder while creating.
-  // |preferred_oem_position|: the preferred position of the OEM folder while
-  //                           creating; if it's invalid then the final position
-  //                           is determined in Ash.
-  // |oem_folder|: the meta data of the existing/created OEM folder.
-  FindOrCreateOemFolder(
-      string oem_folder_name,
-      syncer.mojom.StringOrdinal preferred_oem_position)
-      => (AppListItemMetadata oem_folder);
-  // Resolves the position of the OEM folder.
-  // |preferred_oem_position|: the preferred position of the OEM folder; if it's
-  //                           invalid then the final position is determined in
-  //                           Ash.
-  // |oem_folder|: the meta data of the OEM folder, or null if it doesn't exist.
-  ResolveOemFolderPosition(
-      syncer.mojom.StringOrdinal preferred_oem_position)
-      => (AppListItemMetadata? oem_folder);
-
-  //////////////////////////////////////////////////////////////////////////////
-  // Interfaces for ChromeSearchReult:
-  SetSearchResultMetadata(SearchResultMetadata metadata);
-  SetSearchResultIsInstalling(string result_id, bool is_installing);
-  SetSearchResultPercentDownloaded(string result_id, int32 percent_downloaded);
-  NotifySearchResultItemInstalled(string result_id);
-
-  //////////////////////////////////////////////////////////////////////////////
-  // Interfaces for views:
-  // Dismisses the app list.
-  DismissAppList();
-  // Returns bounds of a rectangle to show an AppInfo dialog.
-  GetAppInfoDialogBounds() => (gfx.mojom.Rect bounds);
-  // Shows the app list and switches to |state|.
-  ShowAppListAndSwitchToState(AppListState state);
-  // Shows the app list.
-  ShowAppList();
-};

chrome/browser/ui/app_list/app_list_client_impl.cc

@@ -10,7 +10,6 @@
 #include <vector>
 
 #include "ash/public/cpp/app_list/app_list_controller.h"
-#include "ash/public/interfaces/constants.mojom.h"
 #include "base/bind.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
@@ -35,11 +34,9 @@
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_navigator.h"
 #include "chrome/browser/ui/browser_navigator_params.h"
-#include "content/public/common/service_manager_connection.h"
 #include "extensions/common/extension.h"
 #include "services/content/public/mojom/constants.mojom.h"
 #include "services/service_manager/public/cpp/connector.h"
-#include "ui/base/models/menu_model.h"
 #include "ui/display/display.h"
 #include "ui/display/screen.h"
 #include "ui/display/types/display_constants.h"
@@ -56,23 +53,9 @@ bool IsTabletMode() {
 
 }  // namespace
 
-AppListClientImpl::MojoRecorderForTest::MojoRecorderForTest() = default;
-AppListClientImpl::MojoRecorderForTest::~MojoRecorderForTest() = default;
-
-int AppListClientImpl::MojoRecorderForTest::Query(int profile_id) const {
-  auto iter = recorder_.find(profile_id);
-  return iter == recorder_.end() ? 0 : iter->second;
-}
-///////////////////////////////////////////////////////////////////////////////
-
-AppListClientImpl::AppListClientImpl() {
-  app_list::AppListController::Get()->SetClient(this);
-
-  // Get the mojo AppListController in Ash.
-  content::ServiceManagerConnection::GetForProcess()
-      ->GetConnector()
-      ->BindInterface(ash::mojom::kServiceName, &app_list_controller_);
-
+AppListClientImpl::AppListClientImpl()
+    : app_list_controller_(app_list::AppListController::Get()) {
+  app_list_controller_->SetClient(this);
   user_manager::UserManager::Get()->AddSessionStateObserver(this);
 
   DCHECK(!g_app_list_client_instance);
@@ -80,7 +63,6 @@ AppListClientImpl::AppListClientImpl() {
 }
 
 AppListClientImpl::~AppListClientImpl() {
-  app_list_controller_.reset();
   SetProfile(nullptr);
 
   user_manager::UserManager::Get()->RemoveSessionStateObserver(this);
@@ -88,8 +70,8 @@ AppListClientImpl::~AppListClientImpl() {
   DCHECK_EQ(this, g_app_list_client_instance);
   g_app_list_client_instance = nullptr;
 
-  if (app_list::AppListController::Get())
-    app_list::AppListController::Get()->SetClient(nullptr);
+  if (app_list_controller_)
+    app_list_controller_->SetClient(nullptr);
 }
 
 // static
@@ -97,6 +79,14 @@ AppListClientImpl* AppListClientImpl::GetInstance() {
   return g_app_list_client_instance;
 }
 
+void AppListClientImpl::OnAppListControllerDestroyed() {
+  // |app_list_controller_| could be released earlier, e.g. starting a kiosk
+  // next session.
+  app_list_controller_ = nullptr;
+  if (current_model_updater_)
+    current_model_updater_->SetActive(false);
+}
+
 void AppListClientImpl::StartSearch(const base::string16& trimmed_query) {
   if (search_controller_) {
     search_controller_->Start(trimmed_query);
@@ -235,9 +225,6 @@ void AppListClientImpl::OnAppListVisibilityChanged(bool visible) {
 void AppListClientImpl::OnFolderCreated(
     int profile_id,
     ash::mojom::AppListItemMetadataPtr item) {
-  if (mojo_recorder_for_test_.get())
-    mojo_recorder_for_test_->Record(profile_id);
-
   auto* requested_model_updater = profile_model_mappings_[profile_id];
   if (!requested_model_updater)
     return;
@@ -386,15 +373,6 @@ AppListModelUpdater* AppListClientImpl::GetModelUpdaterForTest() {
   return current_model_updater_;
 }
 
-void AppListClientImpl::SetUpMojoRecorderForTest() {
-  mojo_recorder_for_test_ = std::make_unique<MojoRecorderForTest>();
-}
-
-int AppListClientImpl::QueryMojoRecorderForTest(int profile_id) {
-  DCHECK(mojo_recorder_for_test_.get());
-  return mojo_recorder_for_test_->Query(profile_id);
-}
-
 void AppListClientImpl::OnTemplateURLServiceChanged() {
   DCHECK(current_model_updater_);
 
@@ -428,8 +406,8 @@ Profile* AppListClientImpl::GetCurrentAppListProfile() const {
   return ChromeLauncherController::instance()->profile();
 }
 
-ash::mojom::AppListController* AppListClientImpl::GetAppListController() const {
-  return app_list_controller_.get();
+app_list::AppListController* AppListClientImpl::GetAppListController() const {
+  return app_list_controller_;
 }
 
 void AppListClientImpl::DismissView() {
@@ -533,7 +511,3 @@ ash::ShelfLaunchSource AppListClientImpl::AppListSourceToLaunchSource(
       return ash::LAUNCH_FROM_UNKNOWN;
   }
 }
-
-void AppListClientImpl::FlushMojoForTesting() {
-  app_list_controller_.FlushForTesting();
-}

chrome/browser/ui/app_list/app_list_client_impl.h

@@ -12,7 +12,6 @@
 
 #include "ash/public/cpp/app_list/app_list_client.h"
 #include "ash/public/cpp/shelf_types.h"
-#include "ash/public/interfaces/app_list.mojom.h"
 #include "base/callback_forward.h"
 #include "base/compiler_specific.h"
 #include "base/macros.h"
@@ -41,29 +40,13 @@ class AppListClientImpl
       public user_manager::UserManager::UserSessionStateObserver,
       public TemplateURLServiceObserver {
  public:
-  class MojoRecorderForTest {
-   public:
-    MojoRecorderForTest();
-    ~MojoRecorderForTest();
-
-    void Record(int profile_id) { recorder_[profile_id]++; }
-
-    int Query(int profile_id) const;
-
-   private:
-    // For each pair in the map, the key is a profile id while the value is the
-    // mojo calling times associated with the particular profile.
-    std::map<int, int> recorder_;
-
-    DISALLOW_COPY_AND_ASSIGN(MojoRecorderForTest);
-  };
-
   AppListClientImpl();
   ~AppListClientImpl() override;
 
   static AppListClientImpl* GetInstance();
 
   // app_list::AppListClient:
+  void OnAppListControllerDestroyed() override;
   void StartSearch(const base::string16& trimmed_query) override;
   void OpenSearchResult(const std::string& result_id,
                         int event_flags,
@@ -145,7 +128,7 @@ class AppListClientImpl
   bool app_list_visible() const { return app_list_visible_; }
 
   // Returns a pointer to control the app list views in ash.
-  ash::mojom::AppListController* GetAppListController() const;
+  app_list::AppListController* GetAppListController() const;
 
   AppListControllerDelegate* GetControllerDelegate();
   Profile* GetCurrentAppListProfile() const;
@@ -154,13 +137,6 @@ class AppListClientImpl
 
   AppListModelUpdater* GetModelUpdaterForTest();
 
-  void SetUpMojoRecorderForTest();
-
-  int QueryMojoRecorderForTest(int profile_id);
-
-  // Flushes all pending mojo call to Ash for testing.
-  void FlushMojoForTesting();
-
  private:
   FRIEND_TEST_ALL_PREFIXES(AppListClientWithProfileTest, CheckDataRace);
 
@@ -199,12 +175,10 @@ class AppListClientImpl
   std::unique_ptr<app_list::SearchController> search_controller_;
   std::unique_ptr<AppSyncUIStateWatcher> app_sync_ui_state_watcher_;
 
-  std::unique_ptr<MojoRecorderForTest> mojo_recorder_for_test_;
-
   ScopedObserver<TemplateURLService, AppListClientImpl>
       template_url_service_observer_{this};
 
-  ash::mojom::AppListControllerPtr app_list_controller_;
+  app_list::AppListController* app_list_controller_ = nullptr;
 
   bool app_list_target_visibility_ = false;
   bool app_list_visible_ = false;

chrome/browser/ui/app_list/app_list_client_impl_browsertest.cc

@@ -246,7 +246,6 @@ IN_PROC_BROWSER_TEST_F(AppListClientSearchResultsBrowserTest,
 
   // Show the app list first, otherwise we won't have a search box to update.
   client->ShowAppList();
-  client->FlushMojoForTesting();
 
   // Currently the search box is empty, so we have no result.
   EXPECT_FALSE(search_controller->GetResultByTitleForTest(title));
@@ -255,16 +254,11 @@ IN_PROC_BROWSER_TEST_F(AppListClientSearchResultsBrowserTest,
   model_updater->UpdateSearchBox(base::ASCIIToUTF16(title),
                                  true /* initiated_by_user */);
 
-  // Ensure everything is done, from Chrome to Ash and backwards.
-  client->FlushMojoForTesting();
   EXPECT_TRUE(search_controller->GetResultByTitleForTest(title));
 
   // Uninstall the extension.
   UninstallExtension(extension->id());
 
-  // Ensure everything is done, from Chrome to Ash and backwards.
-  client->FlushMojoForTesting();
-
   // We cannot find the extension any more.
   EXPECT_FALSE(search_controller->GetResultByTitleForTest(title));
 
@@ -363,124 +357,3 @@ IN_PROC_BROWSER_TEST_F(AppListAppLaunchTest, DemoModeAppLaunchSourceReported) {
       "DemoMode.AppLaunchSource",
       chromeos::DemoSession::AppLaunchSource::kAppList, 1);
 }
-
-class AppListClientWithProfileTest : public InProcessBrowserTest {
- protected:
-  AppListClientWithProfileTest()
-      : account_id1_(
-            AccountId::FromUserEmailGaiaId("test1@example.com", "ID1")),
-        account_id2_(
-            AccountId::FromUserEmailGaiaId("test2@example.com", "ID2")) {}
-  ~AppListClientWithProfileTest() override {}
-
-  static std::unique_ptr<KeyedService> BuildTestSyncService(
-      content::BrowserContext* context) {
-    Profile* profile = Profile::FromBrowserContext(context);
-    return std::make_unique<app_list::AppListSyncableService>(
-        profile, extensions::ExtensionSystem::Get(profile));
-  }
-
-  static std::unique_ptr<KeyedService> BuildTestURLService(
-      content::BrowserContext* context) {
-    return std::make_unique<TemplateURLService>(nullptr, 0);
-  }
-
-  void SetUpCommandLine(base::CommandLine* command_line) override {
-    // Disable the password sync service. Otherwise the test will crash.
-    command_line->AppendSwitchASCII(
-        switches::kDisableSyncTypes,
-        syncer::ModelTypeSetToString(syncer::PASSWORDS));
-  }
-
-  void SetUpOnMainThread() override {
-    user_manager_enabler_ = std::make_unique<user_manager::ScopedUserManager>(
-        std::make_unique<chromeos::FakeChromeUserManager>());
-    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
-
-    profile1_ = BuildProfile(account_id1_);
-    profile2_ = BuildProfile(account_id2_);
-
-    AppListClientImpl::GetInstance()->SetUpMojoRecorderForTest();
-  }
-
-  void TearDownOnMainThread() override {
-    GetFakeUserManager()->RemoveUserFromList(account_id1_);
-    profile1_.reset();
-    profile2_.reset();
-    base::RunLoop().RunUntilIdle();
-    user_manager_enabler_.reset();
-  }
-
-  chromeos::FakeChromeUserManager* GetFakeUserManager() const {
-    return static_cast<chromeos::FakeChromeUserManager*>(
-        user_manager::UserManager::Get());
-  }
-
-  TestingProfile* GetProfile1() { return profile1_.get(); }
-  TestingProfile* GetProfile2() { return profile2_.get(); }
-
- private:
-  std::unique_ptr<TestingProfile> BuildProfile(AccountId account_id) {
-    GetFakeUserManager()->AddUser(account_id);
-    GetFakeUserManager()->LoginUser(account_id);
-
-    TestingProfile::Builder profile_builder;
-    profile_builder.SetPath(
-        temp_dir_.GetPath().AppendASCII(account_id.GetUserEmail()));
-    profile_builder.SetProfileName(account_id.GetUserEmail());
-
-    // Add service factories needed by AppListClientImpl::SetProfile.
-    profile_builder.AddTestingFactory(
-        app_list::AppListSyncableServiceFactory::GetInstance(),
-        base::BindRepeating(&BuildTestSyncService));
-    profile_builder.AddTestingFactory(
-        TemplateURLServiceFactory::GetInstance(),
-        base::BindRepeating(&BuildTestURLService));
-
-    return IdentityTestEnvironmentProfileAdaptor::
-        CreateProfileForIdentityTestEnvironment(profile_builder);
-  }
-
-  AccountId account_id1_;
-  AccountId account_id2_;
-  std::unique_ptr<user_manager::ScopedUserManager> user_manager_enabler_;
-  std::unique_ptr<TestingProfile> profile1_;
-  std::unique_ptr<TestingProfile> profile2_;
-  base::ScopedTempDir temp_dir_;
-};
-
-// Verifies that in multi-profile mode, mojo callings from Ash side to access
-// the app list are handled by the correct AppListModelUpdater. (see
-// https://crbug.com/939755)
-IN_PROC_BROWSER_TEST_F(AppListClientWithProfileTest, CheckDataRace) {
-  AppListClientImpl* client = AppListClientImpl::GetInstance();
-
-  // Emulate that the default profile is the profile 1.
-  client->SetProfile(GetProfile1());
-  int profile_id1 = client->GetModelUpdaterForTest()->model_id();
-
-  // Emulate that the AppListSyncableService adds an item within the folder.
-  // So the AppListClientImpl should receive OnFolderCreated event later.
-  auto* model_updater = client->GetModelUpdaterForTest();
-  std::unique_ptr<ChromeAppListItem> item = std::make_unique<ChromeAppListItem>(
-      GetProfile1(), "item 1", model_updater);
-  item->SetFolderId("folder");
-  model_updater->AddItem(std::move(item));
-
-  // Emulate that the current profile is switched to the profile 2. Wait until
-  // the AppListClientImpl receives the mojo callings from Ash side.
-  client->SetProfile(GetProfile2());
-  client->FlushMojoForTesting();
-
-  int profile_id2 = client->GetModelUpdaterForTest()->model_id();
-
-  // Check the following things:
-  // (1) The model updater associated with the profile 2 should not process the
-  // OnFolderCreated event.
-  // (2) The model updater associated with the profile 1 should process the On-
-  // FolderCreated event.
-  EXPECT_EQ(0, client->QueryMojoRecorderForTest(profile_id2));
-  EXPECT_EQ(1, client->QueryMojoRecorderForTest(profile_id1));
-
-  client->SetProfile(nullptr);
-}

chrome/browser/ui/app_list/app_list_client_interactive_uitest.cc

@@ -18,9 +18,7 @@ IN_PROC_BROWSER_TEST_F(AppListClientInteractiveTest, ShowAndDismiss) {
   AppListClientImpl* client = AppListClientImpl::GetInstance();
   ASSERT_FALSE(client->app_list_visible());
   client->ShowAppList();
-  client->FlushMojoForTesting();
   ASSERT_TRUE(client->app_list_visible());
   client->DismissView();
-  client->FlushMojoForTesting();
   ASSERT_FALSE(client->app_list_target_visibility());
 }

chrome/browser/ui/app_list/chrome_app_list_model_updater.cc

@@ -8,6 +8,7 @@
 #include <utility>
 
 #include "ash/public/cpp/app_list/app_list_config.h"
+#include "ash/public/cpp/app_list/app_list_controller.h"
 #include "base/bind.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/ui/app_list/app_list_client_impl.h"

chrome/browser/ui/app_list/chrome_app_list_model_updater.h

@@ -14,6 +14,10 @@
 #include "base/observer_list.h"
 #include "chrome/browser/ui/app_list/app_list_model_updater.h"
 
+namespace app_list {
+class AppListController;
+}  // namespace app_list
+
 class ChromeAppListItem;
 
 class ChromeAppListModelUpdater : public AppListModelUpdater {
@@ -120,7 +124,7 @@ class ChromeAppListModelUpdater : public AppListModelUpdater {
   std::map<std::string, std::unique_ptr<ChromeAppListItem>> items_;
   Profile* const profile_ = nullptr;
   base::ObserverList<AppListModelUpdaterObserver> observers_;
-  ash::mojom::AppListController* app_list_controller_ = nullptr;
+  app_list::AppListController* app_list_controller_ = nullptr;
   bool search_engine_is_google_ = false;
 
   base::WeakPtrFactory<ChromeAppListModelUpdater> weak_ptr_factory_;