Prevent appearing INVALID position ordinal in sync.

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 555bfbd8b4e574a0268217e7dd5f6b4401a67d69..8117b253ceb0c13fa3a6349fccfcfb87cbcc569d 100644
--- a/chrome/browser/ui/app_list/app_list_syncable_service.cc
+++ b/chrome/browser/ui/app_list/app_list_syncable_service.cc
@@ -567,6 +567,7 @@ AppListSyncableService::CreateSyncItemFromAppItem(AppListItem* app_item) {
     return NULL;
   VLOG(2) << this << " CreateSyncItemFromAppItem:" << app_item->ToDebugString();
   SyncItem* sync_item = CreateSyncItem(app_item->id(), type);
+  DCHECK(app_item->position().IsValid());
   UpdateSyncItemFromAppItem(app_item, sync_item);
   UpdateSyncItemInLocalStorage(profile_, sync_item);
   SendSyncChange(sync_item, SyncChange::ACTION_ADD);
@@ -584,6 +585,8 @@ syncer::StringOrdinal AppListSyncableService::GetPinPosition(
 void AppListSyncableService::SetPinPosition(
     const std::string& app_id,
     const syncer::StringOrdinal& item_pin_ordinal) {
+  // Pin position can be set only after model is built.
+  DCHECK(IsInitialized());
   SyncItem* sync_item = FindSyncItem(app_id);
   SyncChange::SyncChangeType sync_change_type;
   if (sync_item) {
@@ -604,9 +607,15 @@ void AppListSyncableService::AddOrUpdateFromSyncItem(AppListItem* app_item) {
   if (app_item->id() == kOemFolderId)
     return;
 
+  DCHECK(app_item->position().IsValid());
+
   SyncItem* sync_item = FindSyncItem(app_item->id());
   if (sync_item) {
     UpdateAppItemFromSyncItem(sync_item, app_item);
+    if (!sync_item->item_ordinal.IsValid()) {
+      UpdateSyncItem(app_item);
+      VLOG(2) << "Flushing position to sync item " << sync_item;
+    }
     return;
   }
   CreateSyncItemFromAppItem(app_item);
@@ -840,6 +849,33 @@ syncer::SyncMergeResult AppListSyncableService::MergeDataAndStartSyncing(
     change_list.push_back(SyncChange(FROM_HERE,  SyncChange::ACTION_ADD,
                                      GetSyncDataFromSyncItem(sync_item)));
   }
+
+  // Fix items that do not contain valid app list position, required for
+  // builds prior to M53.

[stevenjb, 2017/01/04 20:34:26]

nit: reference crbug.

[khmel, 2017/01/04 21:15:12]

Done.

+  for (const auto& sync_pair : sync_items_) {
+    SyncItem* sync_item = sync_pair.second.get();
+    if (sync_item->item_type != sync_pb::AppListSpecifics::TYPE_APP ||
+        sync_item->item_ordinal.IsValid()) {
+      continue;
+    }
+    const AppListItem* app_item = model_->FindItem(sync_item->item_id);
+    if (app_item) {
+      if (UpdateSyncItemFromAppItem(app_item, sync_item)) {
+        VLOG(1) << "Fixing sync item from existing app " << sync_item;
+        change_list.push_back(SyncChange(FROM_HERE,  SyncChange::ACTION_UPDATE,
+                                         GetSyncDataFromSyncItem(sync_item)));
+      } else {
+        VLOG(1) << "Failed to fix sync item from existing app " << sync_item;

[stevenjb, 2017/01/04 20:34:26]

Maybe use:
sync_item->item_ordinal = syncer::StringOrdinal::CreateInitialOrdinal()
and modify logging?

[khmel, 2017/01/04 21:15:12]

Good point!

+      }
+    } else {
+      sync_item->item_ordinal = syncer::StringOrdinal::CreateInitialOrdinal();
+      VLOG(1) << "Fixing sync item by generate new position ordinal "

[stevenjb, 2017/01/04 20:34:26]

nit: s/generate/generating/
also: ":" separating sync_item in all of these.

[khmel, 2017/01/04 21:15:12]

Done.

+              << sync_item;
+      change_list.push_back(SyncChange(FROM_HERE,  SyncChange::ACTION_UPDATE,
+                                       GetSyncDataFromSyncItem(sync_item)));
+    }
+  }
+
   sync_processor_->ProcessSyncChanges(FROM_HERE, change_list);
 
   HandleUpdateFinished();