arc: Handle position conflict in app list items.

chrome/browser/ui/app_list/app_list_syncable_service.cc

Index: chrome/browser/ui/app_list/app_list_syncable_service.cc
diff --git a/chrome/browser/ui/app_list/app_list_syncable_service.cc b/chrome/browser/ui/app_list/app_list_syncable_service.cc
index fb9385211b40b43a7a7110ae33aa3b1db942fe51..c534fd900d169875516b0d7b5b3657998f05c38b 100644
--- a/chrome/browser/ui/app_list/app_list_syncable_service.cc
+++ b/chrome/browser/ui/app_list/app_list_syncable_service.cc
@@ -1143,18 +1143,21 @@ std::string AppListSyncableService::FindOrCreateOemFolder() {
   if (!oem_folder) {
     std::unique_ptr<AppListFolderItem> new_folder(new AppListFolderItem(
         kOemFolderId, AppListFolderItem::FOLDER_TYPE_OEM));
-    oem_folder =
-        static_cast<AppListFolderItem*>(model_->AddItem(std::move(new_folder)));

[khmel, 2017/03/04 02:57:06]

It is not safe to add oem_folder to model and then call GetOemFolderPos. It may
happen that newly created folder will be placed after web store app and
unintentionally counted.  

     SyncItem* oem_sync_item = FindSyncItem(kOemFolderId);
+    syncer::StringOrdinal oem_position;
     if (oem_sync_item) {
+      DCHECK(oem_sync_item->item_ordinal.IsValid());
       VLOG(1) << "Creating OEM folder from existing sync item: "
                << oem_sync_item->item_ordinal.ToDebugString();
-      model_->SetItemPosition(oem_folder, oem_sync_item->item_ordinal);
+      oem_position = oem_sync_item->item_ordinal;
     } else {
-      model_->SetItemPosition(oem_folder, GetOemFolderPos());
+      oem_position = GetOemFolderPos();
       // Do not create a sync item for the OEM folder here, do it in
       // ResolveFolderPositions() when the item position is finalized.
     }
+    oem_folder =
+        static_cast<AppListFolderItem*>(model_->AddItem(std::move(new_folder)));
+    model_->SetItemPosition(oem_folder, oem_position);
   }
   model_->SetItemName(oem_folder, oem_folder_name_);
   return oem_folder->id();
@@ -1181,26 +1184,42 @@ syncer::StringOrdinal AppListSyncableService::GetOemFolderPos() {
   // stable. TODO(stevenjb): consider explicitly setting the OEM folder location
   // along with the name in ServicesCustomizationDocument::SetOemFolderName().
   AppListItemList* item_list = model_->top_level_item_list();
-  if (item_list->item_count() == 0)
-    return syncer::StringOrdinal();
-
-  size_t oem_index = 0;
-  for (; oem_index < item_list->item_count() - 1; ++oem_index) {
-    AppListItem* cur_item = item_list->item_at(oem_index);
-    if (cur_item->id() == extensions::kWebStoreAppId)
-      break;
+  if (!item_list->item_count()) {
+    LOG(ERROR) << "No top level item was found. "
+               << "Placing OEM folder at the beginning.";
+    return syncer::StringOrdinal::CreateInitialOrdinal();
   }
-  syncer::StringOrdinal oem_ordinal;
-  AppListItem* prev = item_list->item_at(oem_index);
-  if (oem_index + 1 < item_list->item_count()) {
-    AppListItem* next = item_list->item_at(oem_index + 1);
-    oem_ordinal = prev->position().CreateBetween(next->position());
-  } else {
-    oem_ordinal = prev->position().CreateAfter();
+
+  for (size_t i = 0; i < item_list->item_count(); ++i) {
+    const AppListItem* cur_item = item_list->item_at(i);
+    DCHECK(cur_item->position().IsValid());
+    if (cur_item->id() != extensions::kWebStoreAppId)
+      continue;

[stevenjb, 2017/03/06 21:35:15]

This is confusing to me. If you don't like the previous method of finding the
web store index, can we write this something like:

int web_store_idx = GetIndexOf(kWebStoreAppId);
if (web_store_idx < 0)
  log error, return;
// Skip items...

[khmel, 2017/03/06 22:38:15]

Thanks for idea! Hopefully |item_list| already has required helper.

+    // Skip items with the same position.
+    for (size_t j = i + 1; j < item_list->item_count(); ++j) {
+      const AppListItem* next_item = item_list->item_at(j);
+      DCHECK(next_item->position().IsValid());
+      if (!next_item->position().Equals(cur_item->position())) {
+        const syncer::StringOrdinal oem_ordinal =
+            cur_item->position().CreateBetween(next_item->position());
+        VLOG(1) << "Placing OEM Folder at: " << j
+                << " position: " << oem_ordinal.ToDebugString();
+        return oem_ordinal;
+      }
+    }
+
+    const syncer::StringOrdinal oem_ordinal =
+        cur_item->position().CreateAfter();
+    VLOG(1) << "Placing OEM Folder at: " << item_list->item_count()
+            << " position: " << oem_ordinal.ToDebugString();
+    return oem_ordinal;
   }
-  VLOG(1) << "Placing OEM Folder at: " << oem_index
-          << " position: " << oem_ordinal.ToDebugString();
-  return oem_ordinal;
+
+  LOG(ERROR) << "Web store position is not found it top items. "
+             << "Placing OEM folder at the end.";
+  return item_list->item_at(item_list->item_count() - 1)
+      ->position()
+      .CreateAfter();
 }
 
 bool AppListSyncableService::AppIsOem(const std::string& id) {