Reland "app_launcher: Fix OEM folder is not created in new user flow."

app_launcher: Fix OEM folder is not created in new user flow.

This fixes the issue when OEM folder is created empty after new user
flow and OEM items are placed to non-named folder. This also fixes
various crashes when trying to manipulate these items.

Change-Id: Id0fe72be9c47979eb4e910f11682ac9c7f5c8448

Bug: 828209
Test: Manually
Change-Id: Id0fe72be9c47979eb4e910f11682ac9c7f5c8448
Reviewed-on: https://chromium-review.googlesource.com/1017263
Commit-Queue: Yury Khmel <khmel@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551859}

ash/app_list/app_list_controller_impl.cc

@@ -589,7 +589,13 @@ syncer::StringOrdinal AppListControllerImpl::GetOemFolderPos() {
 std::unique_ptr<app_list::AppListItem> AppListControllerImpl::CreateAppListItem(
     AppListItemMetadataPtr metadata) {
   std::unique_ptr<app_list::AppListItem> app_list_item =
-      std::make_unique<app_list::AppListItem>(metadata->id);
+      metadata->is_folder
+          ? std::make_unique<app_list::AppListFolderItem>(
+                metadata->id,
+                metadata->id == kOemFolderId
+                    ? app_list::AppListFolderItem::FOLDER_TYPE_OEM
+                    : app_list::AppListFolderItem::FOLDER_TYPE_NORMAL)
+          : std::make_unique<app_list::AppListItem>(metadata->id);
   app_list_item->SetMetadata(std::move(metadata));
   return app_list_item;
 }

ash/public/cpp/app_list/app_list_types.cc

@@ -6,6 +6,8 @@
 
 namespace ash {
 
+const char kOemFolderId[] = "ddb1da55-d478-4243-8642-56d3041f0263";
+
 ////////////////////////////////////////////////////////////////////////////////
 // SearchResultTag:
 

ash/public/cpp/app_list/app_list_types.h

@@ -13,6 +13,9 @@
 
 namespace ash {
 
+// Id of OEM folder in app list.
+ASH_PUBLIC_EXPORT extern const char kOemFolderId[];
+
 // All possible states of the app list.
 // Note: Do not change the order of these as they are used for metrics.
 enum class AppListState {

chrome/browser/ui/app_list/app_list_syncable_service.cc

@@ -290,10 +290,6 @@ class AppListSyncableService::ModelUpdaterDelegate
 
 // AppListSyncableService
 
-// static
-const char AppListSyncableService::kOemFolderId[] =
-    "ddb1da55-d478-4243-8642-56d3041f0263";
-
 // static
 void AppListSyncableService::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
@@ -443,7 +439,7 @@ AppListSyncableService::GetSyncItem(const std::string& id) const {
 
 void AppListSyncableService::SetOemFolderName(const std::string& name) {
   oem_folder_name_ = name;
-  model_updater_->SetItemName(kOemFolderId, oem_folder_name_);
+  model_updater_->SetItemName(ash::kOemFolderId, oem_folder_name_);
 }
 
 AppListModelUpdater* AppListSyncableService::GetModelUpdater() {
@@ -477,7 +473,7 @@ void AppListSyncableService::AddItem(
   if (AppIsOem(app_item->id())) {
     VLOG(2) << this << ": AddItem to OEM folder: " << sync_item->ToString();
     model_updater_->AddItemToOemFolder(
-        std::move(app_item), FindSyncItem(kOemFolderId), kOemFolderId,
+        std::move(app_item), FindSyncItem(ash::kOemFolderId), ash::kOemFolderId,
         oem_folder_name_, GetPreferredOemFolderPos());
   } else {
     std::string folder_id = sync_item->parent_id;
@@ -558,7 +554,7 @@ void AppListSyncableService::AddOrUpdateFromSyncItem(
     const ChromeAppListItem* app_item) {
   // Do not create a sync item for the OEM folder here, do that in
   // ResolveFolderPositions once the position has been resolved.
-  if (app_item->id() == kOemFolderId)
+  if (app_item->id() == ash::kOemFolderId)
     return;
 
   DCHECK(app_item->position().IsValid());
@@ -567,8 +563,9 @@ void AppListSyncableService::AddOrUpdateFromSyncItem(
   if (sync_item) {
     model_updater_->UpdateAppItemFromSyncItem(
         sync_item,
-        sync_item->item_id != kOemFolderId,  // Don't sync oem folder's name.
-        false);                              // Don't sync its folder here.
+        sync_item->item_id !=
+            ash::kOemFolderId,  // Don't sync oem folder's name.
+        false);                 // Don't sync its folder here.
     if (!sync_item->item_ordinal.IsValid()) {
       UpdateSyncItem(app_item);
       VLOG(2) << "Flushing position to sync item " << sync_item;
@@ -647,10 +644,10 @@ void AppListSyncableService::RemoveUninstalledItem(const std::string& id) {
 void AppListSyncableService::UpdateItem(const ChromeAppListItem* app_item) {
   // Check to see if the item needs to be moved to/from the OEM folder.
   bool is_oem = AppIsOem(app_item->id());
-  if (!is_oem && app_item->folder_id() == kOemFolderId)
+  if (!is_oem && app_item->folder_id() == ash::kOemFolderId)
     model_updater_->MoveItemToFolder(app_item->id(), "");
-  else if (is_oem && app_item->folder_id() != kOemFolderId)
-    model_updater_->MoveItemToFolder(app_item->id(), kOemFolderId);
+  else if (is_oem && app_item->folder_id() != ash::kOemFolderId)
+    model_updater_->MoveItemToFolder(app_item->id(), ash::kOemFolderId);
 }
 
 void AppListSyncableService::RemoveSyncItem(const std::string& id) {
@@ -694,14 +691,15 @@ void AppListSyncableService::ResolveFolderPositions() {
 
     model_updater_->UpdateAppItemFromSyncItem(
         sync_item,
-        sync_item->item_id != kOemFolderId,  // Don't sync oem folder's name.
-        false);                              // Don't sync its folder here.
+        sync_item->item_id !=
+            ash::kOemFolderId,  // Don't sync oem folder's name.
+        false);                 // Don't sync its folder here.
   }
 
   // Move the OEM folder if one exists and we have not synced its position.
-  if (!FindSyncItem(kOemFolderId)) {
+  if (!FindSyncItem(ash::kOemFolderId)) {
     model_updater_->ResolveOemFolderPosition(
-        kOemFolderId, GetPreferredOemFolderPos(),
+        ash::kOemFolderId, GetPreferredOemFolderPos(),
         base::BindOnce(
             [](base::WeakPtr<AppListSyncableService> self,
                ChromeAppListItem* oem_folder) {
@@ -963,8 +961,9 @@ void AppListSyncableService::ProcessNewSyncItem(SyncItem* sync_item) {
       // We don't create new folders here, the model will do that.
       model_updater_->UpdateAppItemFromSyncItem(
           sync_item,
-          sync_item->item_id != kOemFolderId,  // Don't sync oem folder's name.
-          false);                              // It's a folder itself.
+          sync_item->item_id !=
+              ash::kOemFolderId,  // Don't sync oem folder's name.
+          false);                 // It's a folder itself.
       return;
     }
     case sync_pb::AppListSpecifics::TYPE_URL: {
@@ -985,7 +984,7 @@ void AppListSyncableService::ProcessExistingSyncItem(SyncItem* sync_item) {
 
   model_updater_->UpdateAppItemFromSyncItem(
       sync_item,
-      sync_item->item_id != kOemFolderId,  // Don't sync oem folder's name.
+      sync_item->item_id != ash::kOemFolderId,  // Don't sync oem folder's name.
       true);  // The only place where sync can change an item's folder.
 }
 

chrome/browser/ui/app_list/app_list_syncable_service.h

@@ -100,8 +100,6 @@ class AppListSyncableService : public syncer::SyncableService,
 
   ~AppListSyncableService() override;
 
-  static const char kOemFolderId[];
-
   // Registers prefs to support local storage.
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 

chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc

@@ -262,11 +262,9 @@ TEST_F(AppListSyncableServiceTest, OEMFolderForConflictingPos) {
   // OEM item is not top level element.
   ChromeAppListItem* oem_app_item = model_updater()->FindItem(oem_app_id);
   EXPECT_NE(nullptr, oem_app_item);
-  EXPECT_EQ(oem_app_item->folder_id(),
-            app_list::AppListSyncableService::kOemFolderId);
+  EXPECT_EQ(oem_app_item->folder_id(), ash::kOemFolderId);
   // But OEM folder is.
-  ChromeAppListItem* oem_folder =
-      model_updater()->FindItem(app_list::AppListSyncableService::kOemFolderId);
+  ChromeAppListItem* oem_folder = model_updater()->FindItem(ash::kOemFolderId);
   EXPECT_NE(nullptr, oem_folder);
   EXPECT_EQ(oem_folder->folder_id(), "");
 }