ChromeOS: Add test support for ShelfModel synchronization.

Fix tests that break with --ash-enable-shelf-model-synchronization.
(I'm hoping to imminently enable that functionality by default)

Perform some extra cleanup in chrome_launcher_controller_browsertest.cc
(Fix ShelfModel use, avoid Shelf, nix redundant fixture, add another)

Bug: 557406
Test: Automated
Change-Id: If93c8d0c3ec70f4aa427b5eab2b2c43ffc571158
Reviewed-on: https://chromium-review.googlesource.com/691104
Commit-Queue: Michael Wasserman <msw@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505244}

ash/shelf/shelf_controller.cc

@@ -201,9 +201,11 @@ void ShelfController::MoveShelfItem(const ShelfID& id, int32_t index) {
   DCHECK_GT(index, 0) << " Items can not precede the AppList";
   DCHECK_LT(index, model_.item_count()) << " Index out of bounds";
   index = std::min(std::max(index, 1), model_.item_count() - 1);
-  DCHECK_NE(current_index, index) << " The item is already at the given index";
-  if (current_index == index)
+  if (current_index == index) {
+    DVLOG(1) << "The item is already at the given index (" << index << "). "
+             << "This happens when syncing a ShelfModel weight reordering.";
     return;
+  }
   base::AutoReset<bool> reset(&applying_remote_shelf_model_changes_, true);
   model_.Move(current_index, index);
 }

ash/shelf/shelf_controller.h

@@ -39,6 +39,10 @@ class ShelfController : public mojom::ShelfController,
 
   ShelfModel* model() { return &model_; }
 
+  bool should_synchronize_shelf_models() const {
+    return should_synchronize_shelf_models_;
+  }
+
   // mojom::ShelfController:
   void AddObserver(mojom::ShelfObserverAssociatedPtrInfo observer) override;
   void AddShelfItem(int32_t index, const ShelfItem& item) override;

ash/shelf/shelf_controller_unittest.cc

@@ -65,11 +65,11 @@ TEST_F(ShelfControllerTest, IntializesAppListItemDelegate) {
   EXPECT_TRUE(model->GetShelfItemDelegate(ShelfID(kAppListId)));
 }
 
-TEST_F(ShelfControllerTest, ShelfModelChangesInClassicAsh) {
-  if (Shell::GetAshConfig() == Config::MASH)
+TEST_F(ShelfControllerTest, ShelfModelChangesWithoutSync) {
+  ShelfController* controller = Shell::Get()->shelf_controller();
+  if (controller->should_synchronize_shelf_models())
     return;
 
-  ShelfController* controller = Shell::Get()->shelf_controller();
   TestShelfObserver observer;
   mojom::ShelfObserverAssociatedPtr observer_ptr;
   mojo::AssociatedBinding<mojom::ShelfObserver> binding(
@@ -77,12 +77,12 @@ TEST_F(ShelfControllerTest, ShelfModelChangesInClassicAsh) {
   controller->AddObserver(observer_ptr.PassInterface());
 
   // The ShelfModel should be initialized with a single item for the AppList.
-  // In classic ash, the observer should not be notified of ShelfModel changes.
+  // Without syncing, the observer should not be notified of ShelfModel changes.
   EXPECT_EQ(1, controller->model()->item_count());
   EXPECT_EQ(0u, observer.added_count());
   EXPECT_EQ(0u, observer.removed_count());
 
-  // Add a ShelfModel item; |observer| should not be notified in classic ash.
+  // Add a ShelfModel item; |observer| should not be notified without sync.
   ShelfItem item;
   item.type = TYPE_PINNED_APP;
   item.id = ShelfID("foo");
@@ -92,7 +92,7 @@ TEST_F(ShelfControllerTest, ShelfModelChangesInClassicAsh) {
   EXPECT_EQ(0u, observer.added_count());
   EXPECT_EQ(0u, observer.removed_count());
 
-  // Remove a ShelfModel item; |observer| should not be notified in classic ash.
+  // Remove a ShelfModel item; |observer| should not be notified without sync.
   controller->model()->RemoveItemAt(index);
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(1, controller->model()->item_count());
@@ -100,11 +100,11 @@ TEST_F(ShelfControllerTest, ShelfModelChangesInClassicAsh) {
   EXPECT_EQ(0u, observer.removed_count());
 }
 
-TEST_F(ShelfControllerTest, ShelfModelChangesInMash) {
-  if (Shell::GetAshConfig() != Config::MASH)
+TEST_F(ShelfControllerTest, ShelfModelChangesWithSync) {
+  ShelfController* controller = Shell::Get()->shelf_controller();
+  if (!controller->should_synchronize_shelf_models())
     return;
 
-  ShelfController* controller = Shell::Get()->shelf_controller();
   TestShelfObserver observer;
   mojom::ShelfObserverAssociatedPtr observer_ptr;
   mojo::AssociatedBinding<mojom::ShelfObserver> binding(
@@ -113,12 +113,12 @@ TEST_F(ShelfControllerTest, ShelfModelChangesInMash) {
   base::RunLoop().RunUntilIdle();
 
   // The ShelfModel should be initialized with a single item for the AppList.
-  // In mash, the observer is immediately notified of existing shelf items.
+  // When syncing, the observer is immediately notified of existing shelf items.
   EXPECT_EQ(1, controller->model()->item_count());
   EXPECT_EQ(1u, observer.added_count());
   EXPECT_EQ(0u, observer.removed_count());
 
-  // Add a ShelfModel item; |observer| should be notified in mash.
+  // Add a ShelfModel item; |observer| should be notified when syncing.
   ShelfItem item;
   item.type = TYPE_PINNED_APP;
   item.id = ShelfID("foo");
@@ -128,7 +128,7 @@ TEST_F(ShelfControllerTest, ShelfModelChangesInMash) {
   EXPECT_EQ(2u, observer.added_count());
   EXPECT_EQ(0u, observer.removed_count());
 
-  // Remove a ShelfModel item; |observer| should be notified in mash.
+  // Remove a ShelfModel item; |observer| should be notified when syncing.
   controller->model()->RemoveItemAt(index);
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(1, controller->model()->item_count());

chrome/browser/ui/ash/launcher/arc_app_launcher_browsertest.cc

@@ -256,7 +256,7 @@ class ArcAppLauncherBrowserTest : public ExtensionBrowserTest {
   }
 
   ash::ShelfItemDelegate* GetShelfItemDelegate(const std::string& id) {
-    ash::ShelfModel* model = ash::Shell::Get()->shelf_model();
+    auto* model = ChromeLauncherController::instance()->shelf_model();
     return model->GetShelfItemDelegate(ash::ShelfID(id));
   }
 

chrome/browser/ui/window_sizer/window_sizer_ash_uitest.cc

@@ -69,6 +69,8 @@ void OpenBrowserUsingShelfOnRootWindow(aura::Window* root_window) {
   center.Offset(- origin.x(), - origin.y());
   generator.MoveMouseTo(center);
   generator.ClickLeftButton();
+  // Spin a run loop so Ash can notify Chrome of item selection for handling.
+  base::RunLoop().RunUntilIdle();
 }
 
 }  // namespace