Clean up single item folder after initial sync.

App list sync may have some edge cases that lead to the creation of
single item folder which is not legitimate for launcher. When user signs
in a new session, app list sync service will download all the sync data
from sync backend and merge them with the cached sync data in local
storage. We will detect the single item folder sync items and clean them
up right after the initial merging completes in AppListSyncableService.
Therefore, the single item folder should go away from the launcher UI
after user starts a new session.

This does not fix the root cause of issues that lead to the creation of
the single item folder, but makes it a little better with a remedy. We
will also fix root cause of the issue for the cases we can repro in
separate changelists.

Bug: 1082530
Change-Id: I95098e0c97eb3faaf8d90d6654d9f1ca8a60094c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2219176Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Jenny Zhang <jennyz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#773777}

chrome/browser/ui/app_list/app_list_syncable_service.cc

@@ -20,6 +20,7 @@
 #include "chrome/browser/apps/app_service/app_service_proxy_factory.h"
 #include "chrome/browser/chromeos/arc/arc_util.h"
 #include "chrome/browser/chromeos/crostini/crostini_features.h"
+#include "chrome/browser/chromeos/crostini/crostini_util.h"
 #include "chrome/browser/chromeos/file_manager/app_id.h"
 #include "chrome/browser/chromeos/web_applications/default_web_app_ids.h"
 #include "chrome/browser/extensions/extension_service.h"
@@ -193,6 +194,13 @@ bool IsOsSettingsApp(const std::string& app_id) {
   return app_id == chromeos::default_web_apps::kOsSettingsAppId;
 }
 
+bool IsSystemCreatedSyncFolder(AppListSyncableService::SyncItem* folder_item) {
+  if (folder_item->item_type != sync_pb::AppListSpecifics::TYPE_FOLDER)
+    return false;
+  return (folder_item->item_id == ash::kOemFolderId ||
+          folder_item->item_id == crostini::kCrostiniFolderId);
+}
+
 }  // namespace
 
 // AppListSyncableService::ScopedModelUpdaterFactoryForTest
@@ -404,7 +412,7 @@ void AppListSyncableService::BuildModel() {
 
   app_service_apps_builder_->Initialize(this, profile_, model_updater_.get());
 
-  HandleUpdateFinished();
+  HandleUpdateFinished(false /* clean_up_after_init_sync */);
 
   if (wait_until_ready_to_sync_cb_)
     std::move(wait_until_ready_to_sync_cb_).Run();
@@ -498,7 +506,7 @@ void AppListSyncableService::ApplyAppAttributes(
   SendSyncChange(item, SyncChange::ACTION_UPDATE);
   ProcessExistingSyncItem(item);
 
-  HandleUpdateFinished();
+  HandleUpdateFinished(false /* clean_up_after_init_sync */);
 }
 
 void AppListSyncableService::SetOemFolderName(const std::string& name) {
@@ -520,17 +528,70 @@ void AppListSyncableService::HandleUpdateStarted() {
   model_updater_observer_.reset();
 }
 
-void AppListSyncableService::HandleUpdateFinished() {
+void AppListSyncableService::HandleUpdateFinished(
+    bool clean_up_after_init_sync) {
   // Processing an update may create folders without setting their positions.
   // Resolve them now.
   ResolveFolderPositions();
 
+  if (clean_up_after_init_sync) {
+    PruneEmptySyncFolders();
+    CleanUpSingleItemSyncFolder();
+  }
+
   // Resume or start observing app list model changes.
   model_updater_observer_ = std::make_unique<ModelUpdaterObserver>(this);
 
   NotifyObserversSyncUpdated();
 }
 
+void AppListSyncableService::CleanUpSingleItemSyncFolder() {
+  std::vector<std::string> ids_to_be_deleted;
+  for (auto iter = sync_items_.begin(); iter != sync_items_.end();) {
+    SyncItem* sync_item = (iter++)->second.get();
+    std::string child_item_id;
+    SyncItem* child_item = GetOnlyChildOfUserCreatedFolder(sync_item);
+    if (child_item) {
+      // Move the single child item out of folder and put at the same relative
+      // location as the folder.
+      child_item->item_ordinal = sync_item->item_ordinal;
+      child_item->parent_id = "";
+      UpdateSyncItemInLocalStorage(profile_, child_item);
+      SendSyncChange(child_item, SyncChange::ACTION_UPDATE);
+      // Update the app list model updater for the sync change.
+      ProcessExistingSyncItem(child_item);
+
+      // Remember the id of the folder item to be deleted.
+      ids_to_be_deleted.push_back(sync_item->item_id);
+    }
+  }
+
+  // Remove the empty folder items.
+  for (auto id : ids_to_be_deleted)
+    DeleteSyncItem(id);
+}
+
+AppListSyncableService::SyncItem*
+AppListSyncableService::GetOnlyChildOfUserCreatedFolder(SyncItem* sync_item) {
+  if (sync_item->item_type != sync_pb::AppListSpecifics::TYPE_FOLDER ||
+      IsSystemCreatedSyncFolder(sync_item))
+    return nullptr;
+
+  const std::string& folder_id = sync_item->item_id;
+  int child_count = 0;
+  SyncItem* child_item = nullptr;
+  for (auto iter = sync_items_.begin(); iter != sync_items_.end();) {
+    SyncItem* item = (iter++)->second.get();
+    if (item->parent_id == folder_id) {
+      ++child_count;
+      child_item = item;
+      if (child_count > 1)
+        return nullptr;
+    }
+  }
+  return child_item;
+}
+
 void AppListSyncableService::AddItem(
     std::unique_ptr<ChromeAppListItem> app_item) {
   SyncItem* sync_item = FindOrAddSyncItem(app_item.get());
@@ -937,7 +998,7 @@ AppListSyncableService::MergeDataAndStartSyncing(
 
   sync_processor_->ProcessSyncChanges(FROM_HERE, change_list);
 
-  HandleUpdateFinished();
+  HandleUpdateFinished(true /* clean_up_after_init_sync */);
 
   // Check if already signaled since unit tests make multiple calls.
   if (!on_initialized_.is_signaled()) {
@@ -991,7 +1052,7 @@ base::Optional<syncer::ModelError> AppListSyncableService::ProcessSyncChanges(
     }
   }
 
-  HandleUpdateFinished();
+  HandleUpdateFinished(false /* clean_up_after_init_sync */);
 
   return base::nullopt;
 }

chrome/browser/ui/app_list/app_list_syncable_service.h

@@ -284,7 +284,17 @@ class AppListSyncableService : public syncer::SyncableService,
 
   // Handles model update start/finish.
   void HandleUpdateStarted();
-  void HandleUpdateFinished();
+  void HandleUpdateFinished(bool clean_up_after_init_sync);
+
+  // Cleans up the folder sync item with only one item in it.
+  // There are some edge cases with synch which will create a folder with only
+  // one item in it, which is not legitimate and the folder should be removed.
+  // We will find such folders after the initial sync and clean them up.
+  void CleanUpSingleItemSyncFolder();
+
+  // Returns child item if |sync_item| is a user created folder with only one
+  // child item in it; otherwise, returns nullptr.
+  SyncItem* GetOnlyChildOfUserCreatedFolder(SyncItem* sync_item);
 
   // Returns true if extension service is ready.
   bool IsExtensionServiceReady() const;

chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc

@@ -198,6 +198,15 @@ bool AreAllAppAtributesNotEqualInAppList(const ChromeAppListItem* item1,
          !item1->position().EqualsOrBothInvalid(item2->position());
 }
 
+std::string GetLastPositionString() {
+  static syncer::StringOrdinal last_position;
+  if (!last_position.IsValid())
+    last_position = syncer::StringOrdinal::CreateInitialOrdinal();
+  else
+    last_position = last_position.CreateAfter();
+  return last_position.ToDebugString();
+}
+
 }  // namespace
 
 class AppListSyncableServiceTest : public AppListTestBase {
@@ -620,16 +629,19 @@ TEST_F(AppListSyncableServiceTest, InitialMergeAndUpdate_BadData) {
 }
 
 TEST_F(AppListSyncableServiceTest, PruneEmptySyncFolder) {
-  // Add a folder item and an item that is parented to the folder item.
+  // Add a folder item and two items that are parented to the folder item.
   const std::string kFolderItemId = GenerateId("folder_item_id");
-  const std::string kItemId = GenerateId("item_id");
+  const std::string kItemId1 = GenerateId("item_id_1");
+  const std::string kItemId2 = GenerateId("item_id_2");
 
   syncer::SyncDataList sync_list;
   sync_list.push_back(CreateAppRemoteData(
       kFolderItemId, "folder_item_name", kParentId(), "ordinal", "pinordinal",
       sync_pb::AppListSpecifics_AppListItemType_TYPE_FOLDER));
-  sync_list.push_back(CreateAppRemoteData(kItemId, "item_name", kFolderItemId,
-                                          "ordinal", "pinordinal"));
+  sync_list.push_back(CreateAppRemoteData(kItemId1, "item1_name", kFolderItemId,
+                                          "ordinal1", "pinordinal1"));
+  sync_list.push_back(CreateAppRemoteData(kItemId2, "item2_name", kFolderItemId,
+                                          "ordinal2", "pinordinal2"));
 
   app_list_syncable_service()->MergeDataAndStartSyncing(
       syncer::APP_LIST, sync_list,
@@ -638,14 +650,82 @@ TEST_F(AppListSyncableServiceTest, PruneEmptySyncFolder) {
   content::RunAllTasksUntilIdle();
 
   ASSERT_TRUE(GetSyncItem(kFolderItemId));
-  ASSERT_TRUE(GetSyncItem(kItemId));
+  ASSERT_TRUE(GetSyncItem(kItemId1));
+  ASSERT_TRUE(GetSyncItem(kItemId2));
+
+  // Remove one of the child item, the folder still has one item in it.
+  app_list_syncable_service()->RemoveItem(kItemId1);
+  content::RunAllTasksUntilIdle();
+
+  ASSERT_TRUE(GetSyncItem(kFolderItemId));
+  ASSERT_FALSE(GetSyncItem(kItemId1));
+  ASSERT_TRUE(GetSyncItem(kItemId2));
 
-  // Remove the item, the empty folder item should be removed as well.
-  app_list_syncable_service()->RemoveItem(kItemId);
+  // Remove the other child item, the empty folder should be removed as well.
+  app_list_syncable_service()->RemoveItem(kItemId2);
   content::RunAllTasksUntilIdle();
 
   ASSERT_FALSE(GetSyncItem(kFolderItemId));
-  ASSERT_FALSE(GetSyncItem(kItemId));
+  ASSERT_FALSE(GetSyncItem(kItemId2));
+}
+
+TEST_F(AppListSyncableServiceTest,
+       CleanUpSingleItemSyncFolderAfterInitialMerge) {
+  syncer::SyncDataList sync_list;
+  // Add a top level item.
+  const std::string kTopItem = GenerateId("top_item_id");
+  sync_list.push_back(CreateAppRemoteData(
+      kTopItem, "top_item_name", "", GetLastPositionString(), "pinordinal"));
+
+  // Add a single app folder item with only one child app item in it.
+  const std::string kFolderId1 = GenerateId("folder_id_1");
+  const std::string kChildItemId1 = GenerateId("child_item_id_1");
+  sync_list.push_back(CreateAppRemoteData(
+      kFolderId1, "folder_1_name", "", GetLastPositionString(), "pinordinal",
+      sync_pb::AppListSpecifics_AppListItemType_TYPE_FOLDER));
+  sync_list.push_back(CreateAppRemoteData(kChildItemId1, "child_item_1_name",
+                                          kFolderId1, GetLastPositionString(),
+                                          "pinordinal"));
+
+  // Add a regular folder with two app items in it.
+  const std::string kFolderId2 = GenerateId("folder_id_2");
+  const std::string kChildItemId2 = GenerateId("child_item_id_2");
+  const std::string kChildItemId3 = GenerateId("child_item_id_3");
+  sync_list.push_back(CreateAppRemoteData(
+      kFolderId2, "folder_2_name", "", GetLastPositionString(), "pinordinal",
+      sync_pb::AppListSpecifics_AppListItemType_TYPE_FOLDER));
+  sync_list.push_back(CreateAppRemoteData(kChildItemId2, "child_item_2_name",
+                                          kFolderId2, GetLastPositionString(),
+                                          "pinordinal"));
+  sync_list.push_back(CreateAppRemoteData(kChildItemId3, "child_item_3_name",
+                                          kFolderId2, GetLastPositionString(),
+                                          "pinordinal"));
+
+  app_list_syncable_service()->MergeDataAndStartSyncing(
+      syncer::APP_LIST, sync_list,
+      std::make_unique<syncer::FakeSyncChangeProcessor>(),
+      std::make_unique<syncer::SyncErrorFactoryMock>());
+  content::RunAllTasksUntilIdle();
+
+  // After the initial sync data is merged, the single item folder is expected
+  // to be cleaned up.
+  ASSERT_FALSE(GetSyncItem(kFolderId1));
+  // The child item in the folder should be moved to the top level.
+  ASSERT_TRUE(GetSyncItem(kChildItemId1));
+  EXPECT_EQ("", GetSyncItem(kChildItemId1)->parent_id);
+  EXPECT_TRUE(
+      GetSyncItem(kChildItemId1)
+          ->item_ordinal.GreaterThan(GetSyncItem(kTopItem)->item_ordinal));
+
+  // Sync items should be created for regular folder.
+  ASSERT_TRUE(GetSyncItem(kFolderId2));
+  ASSERT_TRUE(GetSyncItem(kChildItemId2));
+  EXPECT_EQ(kFolderId2, GetSyncItem(kChildItemId2)->parent_id);
+  ASSERT_TRUE(GetSyncItem(kChildItemId3));
+  EXPECT_EQ(kFolderId2, GetSyncItem(kChildItemId3)->parent_id);
+  EXPECT_TRUE(
+      GetSyncItem(kChildItemId1)
+          ->item_ordinal.LessThan(GetSyncItem(kFolderId2)->item_ordinal));
 }
 
 TEST_F(AppListSyncableServiceTest, AddPageBreakItems) {