Prevent appearing INVALID position ordinal in sync.

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 549 matching lines @@
   return CreateSyncItemFromAppItem(app_item);
 }
 
 AppListSyncableService::SyncItem*
 AppListSyncableService::CreateSyncItemFromAppItem(AppListItem* app_item) {
   sync_pb::AppListSpecifics::AppListItemType type;
   if (!GetAppListItemType(app_item, &type))
     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);
   return sync_item;
 }
 
 syncer::StringOrdinal AppListSyncableService::GetPinPosition(
     const std::string& app_id) {
   SyncItem* sync_item = FindSyncItem(app_id);
   if (!sync_item)
     return syncer::StringOrdinal();
   return sync_item->item_pin_ordinal;
 }
 
 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) {
     sync_change_type = SyncChange::ACTION_UPDATE;
   } else {
     sync_item = CreateSyncItem(app_id, sync_pb::AppListSpecifics::TYPE_APP);
     sync_change_type = SyncChange::ACTION_ADD;
   }
 
   sync_item->item_pin_ordinal = item_pin_ordinal;
   UpdateSyncItemInLocalStorage(profile_, sync_item);
   SendSyncChange(sync_item, sync_change_type);
 }
 
 void AppListSyncableService::AddOrUpdateFromSyncItem(AppListItem* 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)
     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);
 }
 
 bool AppListSyncableService::RemoveDefaultApp(AppListItem* item,
                                               SyncItem* sync_item) {
   CHECK_EQ(sync_item->item_type,
            sync_pb::AppListSpecifics::TYPE_REMOVE_DEFAULT_APP);
 
@@ skipping 213 matching lines @@
     SyncItem* sync_item = FindSyncItem(*iter);
     // Sync can cause an item to change folders, causing an unsynced folder
     // item to be removed.
     if (!sync_item)
       continue;
     VLOG(2) << this << " -> SYNC ADD: " << sync_item->ToString();
     UpdateSyncItemInLocalStorage(profile_, sync_item);
     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();
 
   return result;
 }
 
 void AppListSyncableService::StopSyncing(syncer::ModelType type) {
   DCHECK_EQ(type, syncer::APP_LIST);
 
@@ skipping 339 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