arc: Handle position conflict in app list items.

chrome/browser/ui/app_list/app_list_syncable_service.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/app_list/app_list_syncable_service.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/macros.h"
@@ skipping 1125 matching lines @@
           GetDriveAppIdFromSyncId(item_id));
     }
   }
 }
 
 std::string AppListSyncableService::FindOrCreateOemFolder() {
   AppListFolderItem* oem_folder = model_->FindFolderItem(kOemFolderId);
   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();
 }
 
 syncer::StringOrdinal AppListSyncableService::GetOemFolderPos() {
   VLOG(1) << "GetOemFolderPos: " << first_app_list_sync_;
   if (!first_app_list_sync_) {
     VLOG(1) << "Sync items exist, placing OEM folder at end.";
     syncer::StringOrdinal last;
     for (const auto& sync_pair : sync_items_) {
       SyncItem* sync_item = sync_pair.second.get();
       if (sync_item->item_ordinal.IsValid() &&
           (!last.IsValid() || sync_item->item_ordinal.GreaterThan(last))) {
         last = sync_item->item_ordinal;
       }
     }
     if (last.IsValid())
       return last.CreateAfter();
   }
 
   // Place the OEM folder just after the web store, which should always be
   // followed by a pre-installed app (e.g. Search), so the poosition should be
   // 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();
+  if (!item_list->item_count()) {
+    LOG(ERROR) << "No top level item was found. "
+               << "Placing OEM folder at the beginning.";
+    return syncer::StringOrdinal::CreateInitialOrdinal();
+  }
 
-  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;
+  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;
   }
-  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();
-  }
-  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) {
 #if defined(OS_CHROMEOS)
   const ArcAppListPrefs* arc_prefs = ArcAppListPrefs::Get(profile_);
   if (arc_prefs && arc_prefs->IsOem(id))
     return true;
 #endif
 
   if (!extension_system_->extension_service())
@@ skipping 11 matching lines @@
     res += " { " + item_name + " }";
     res += " [" + item_ordinal.ToDebugString() + "]";
     if (!parent_id.empty())
       res += " <" + parent_id.substr(0, 8) + ">";
     res += " [" + item_pin_ordinal.ToDebugString() + "]";
   }
   return res;
 }
 
 }  // namespace app_list