Protect AppListItemList Add/Remove and fix sync bugs

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 "base/command_line.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/app_list/app_list_service.h"
 #include "chrome/browser/ui/app_list/extension_app_item.h"
 #include "chrome/browser/ui/app_list/extension_app_model_builder.h"
 #include "chrome/browser/ui/host_desktop.h"
 #include "chrome/common/chrome_switches.h"
 #include "content/public/browser/notification_source.h"
 #include "extensions/browser/extension_prefs.h"
 #include "sync/api/sync_change_processor.h"
 #include "sync/api/sync_data.h"
 #include "sync/api/sync_merge_result.h"
 #include "sync/protocol/sync.pb.h"
 #include "ui/app_list/app_list_folder_item.h"
 #include "ui/app_list/app_list_item.h"
 #include "ui/app_list/app_list_model.h"
+#include "ui/app_list/app_list_model_observer.h"
+#include "ui/app_list/app_list_switches.h"
 
 using syncer::SyncChange;
 
 namespace app_list {
 
 namespace {
 
 bool SyncAppListEnabled() {
   return CommandLine::ForCurrentProcess()->HasSwitch(
-      switches::kEnableSyncAppList);
+      ::switches::kEnableSyncAppList);
 }
 
 void UpdateSyncItemFromSync(const sync_pb::AppListSpecifics& specifics,
                             AppListSyncableService::SyncItem* item) {
   DCHECK_EQ(item->item_id, specifics.item_id());
   item->item_type = specifics.item_type();
   item->item_name = specifics.item_name();
   item->parent_id = specifics.parent_id();
   if (!specifics.page_ordinal().empty())
     item->page_ordinal = syncer::StringOrdinal(specifics.page_ordinal());
@@ skipping 70 matching lines @@
 AppListSyncableService::SyncItem::SyncItem(
     const std::string& id,
     sync_pb::AppListSpecifics::AppListItemType type)
     : item_id(id),
       item_type(type) {
 }
 
 AppListSyncableService::SyncItem::~SyncItem() {
 }
 
-// AppListSyncableService::ItemListObserver
+// AppListSyncableService::ModelObserver
 
-class AppListSyncableService::ItemListObserver
-    : public AppListItemListObserver {
+class AppListSyncableService::ModelObserver : public AppListModelObserver {
  public:
-  explicit ItemListObserver(AppListSyncableService* owner) : owner_(owner) {
-    owner_->model()->item_list()->AddObserver(this);
+  explicit ModelObserver(AppListSyncableService* owner)
+      : owner_(owner),
+        processing_sync_changes_(false) {
+    owner_->model()->AddObserver(this);
   }
 
-  virtual ~ItemListObserver() {
-    owner_->model()->item_list()->RemoveObserver(this);
+  virtual ~ModelObserver() {
+    owner_->model()->RemoveObserver(this);
+  }
+
+  // Called by owner to indicate that incoming sync changes are being processed.
+  // Changes to the model will be ignored while this is true (since they
+  // originated from sync changes).
+  void set_processing_sync_changes(bool processing_sync_changes) {
+    processing_sync_changes_ = processing_sync_changes;

[xiyuan, 2014/01/28 05:58:29]

Can we also Add/RemoveObserer here? If we can, we don't need to test the flag in
the notification handlers below.

[stevenjb, 2014/01/28 18:28:08]

I didn't go that route for two reasons:
1) This makes it easier to debug, and possibly modify the logic later.
2) Adding / Removing observers has additional overhead.

I can change to adding/removing as an observer if you feel that would be better
/ more clear.

   }
 
  private:
-  // AppListItemListObserver
-  virtual void OnListItemAdded(size_t index, AppListItem* item) OVERRIDE {
+  // AppListModelObserver
+  virtual void OnAppListItemAdded(AppListItem* item) OVERRIDE {
+    if (processing_sync_changes_)
+      return;
+    DVLOG(2) << owner_ << " OnAppListItemAdded: " << item->ToDebugString();
     owner_->AddOrUpdateFromSyncItem(item);
   }
 
-  virtual void OnListItemRemoved(size_t index, AppListItem* item) OVERRIDE {
+  virtual void OnAppListItemWillBeDeleted(AppListItem* item) OVERRIDE {
+    if (processing_sync_changes_)
+      return;
+    DVLOG(2) << owner_ << " OnAppListItemDeleted: " << item->ToDebugString();
     owner_->RemoveSyncItem(item->id());
   }
 
-  virtual void OnListItemMoved(size_t from_index,
-                               size_t to_index,
-                               AppListItem* item) OVERRIDE {
+  virtual void OnAppListItemUpdated(AppListItem* item) OVERRIDE {
+    if (processing_sync_changes_)
+      return;
+    DVLOG(2) << owner_ << " OnAppListItemUpdated: " << item->ToDebugString();
     owner_->UpdateSyncItem(item);
   }
 
   AppListSyncableService* owner_;
+  bool processing_sync_changes_;
 
-  DISALLOW_COPY_AND_ASSIGN(ItemListObserver);
+  DISALLOW_COPY_AND_ASSIGN(ModelObserver);
 };
 
 // AppListSyncableService
 
 AppListSyncableService::AppListSyncableService(
     Profile* profile,
     extensions::ExtensionSystem* extension_system)
     : profile_(profile),
       extension_system_(extension_system),
       model_(new AppListModel) {
   if (!extension_system || !extension_system->extension_service()) {
     LOG(WARNING) << "AppListSyncableService created with no ExtensionService";
     return;
   }
 
-  if (SyncAppListEnabled())
-    item_list_observer_.reset(new ItemListObserver(this));
-
+  // Note: model_observer_ is constructed after the initial sync changes are
+  // received in MergeDataAndStartSyncing(). Changes to the model before that
+  // will be synced after the initial sync occurs.
   if (extension_system->extension_service()->is_ready()) {
     BuildModel();
     return;
   }
 
   // The extensions for this profile have not yet all been loaded.
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSIONS_READY,
                  content::Source<Profile>(profile));
 }
 
 AppListSyncableService::~AppListSyncableService() {
   // Remove observers.
-  item_list_observer_.reset();
+  model_observer_.reset();
 
   STLDeleteContainerPairSecondPointers(sync_items_.begin(), sync_items_.end());
 }
 
 void AppListSyncableService::BuildModel() {
   // For now, use the AppListControllerDelegate associated with the native
   // desktop. TODO(stevenjb): Remove ExtensionAppModelBuilder controller
   // dependency and move the dependent methods from AppListControllerDelegate
   // to an extension service delegate associated with this class.
   AppListControllerDelegate* controller = NULL;
@@ skipping 26 matching lines @@
 
 const AppListSyncableService::SyncItem*
 AppListSyncableService::GetSyncItem(const std::string& id) const {
   SyncItemMap::const_iterator iter = sync_items_.find(id);
   if (iter != sync_items_.end())
     return iter->second;
   return NULL;
 }
 
 void AppListSyncableService::AddItem(AppListItem* app_item) {
-  SyncItem* sync_item = AddOrUpdateSyncItem(app_item);
+  SyncItem* sync_item = FindOrAddSyncItem(app_item);
   if (!sync_item)
     return;  // Item is not valid.
 
   DVLOG(1) << this << ": AddItem: " << sync_item->ToString();
-
-  // Add the item to the model if necessary.
-  if (!model_->item_list()->FindItem(app_item->id()))
-    model_->item_list()->AddItem(app_item);
-  else
-    model_->item_list()->SetItemPosition(app_item, sync_item->item_ordinal);
+  model_->AddItem(app_item);
 }
 
-AppListSyncableService::SyncItem* AppListSyncableService::AddOrUpdateSyncItem(
+AppListSyncableService::SyncItem* AppListSyncableService::FindOrAddSyncItem(
     AppListItem* app_item) {
   const std::string& item_id = app_item->id();
   if (item_id.empty()) {
     LOG(ERROR) << "AppListItem item with empty ID";
     return NULL;
   }
   SyncItem* sync_item = FindSyncItem(item_id);
   if (sync_item) {
-    // If there is an existing, non-REMOVE_DEFAULT entry, update it.
+    // If there is an existing, non-REMOVE_DEFAULT entry, return it.
     if (sync_item->item_type !=
         sync_pb::AppListSpecifics::TYPE_REMOVE_DEFAULT_APP) {
       DVLOG(2) << this << ": AddItem already exists: " << sync_item->ToString();
-      UpdateSyncItem(app_item);
       return sync_item;
     }
 
     if (RemoveDefaultApp(app_item, sync_item))
       return NULL;
 
     // Fall through. The REMOVE_DEFAULT_APP entry has been deleted, now a new
     // App entry can be added.
   }
 
@@ skipping 64 matching lines @@
   bool changed = UpdateSyncItemFromAppItem(app_item, sync_item);
   if (!changed) {
     DVLOG(2) << this << " - Update: SYNC NO CHANGE: " << sync_item->ToString();
     return;
   }
   SendSyncChange(sync_item, SyncChange::ACTION_UPDATE);
 }
 
 void AppListSyncableService::RemoveItem(const std::string& id) {
   RemoveSyncItem(id);
-  model_->item_list()->DeleteItem(id);
+  model_->DeleteItem(id);
 }
 
 void AppListSyncableService::RemoveSyncItem(const std::string& id) {
   DVLOG(2) << this << ": RemoveSyncItem: " << id.substr(0, 8);
   SyncItemMap::iterator iter = sync_items_.find(id);
   if (iter == sync_items_.end()) {
     DVLOG(2) << this << " : RemoveSyncItem: No Item.";
     return;
   }
 
@@ skipping 71 matching lines @@
   syncer::SyncChangeList change_list;
   for (std::set<std::string>::iterator iter = unsynced_items.begin();
        iter != unsynced_items.end(); ++iter) {
     SyncItem* sync_item = FindSyncItem(*iter);
     DVLOG(2) << this << " -> SYNC ADD: " << sync_item->ToString();
     change_list.push_back(SyncChange(FROM_HERE,  SyncChange::ACTION_ADD,
                                      GetSyncDataFromSyncItem(sync_item)));
   }
   sync_processor_->ProcessSyncChanges(FROM_HERE, change_list);
 
+  // Start observing app list changes.
+  model_observer_.reset(new ModelObserver(this));
+
   return result;
 }
 
 void AppListSyncableService::StopSyncing(syncer::ModelType type) {
   DCHECK_EQ(type, syncer::APP_LIST);
 
   sync_processor_.reset();
   sync_error_handler_.reset();
 }
 
@@ skipping 13 matching lines @@
 
 syncer::SyncError AppListSyncableService::ProcessSyncChanges(
     const tracked_objects::Location& from_here,
     const syncer::SyncChangeList& change_list) {
   if (!sync_processor_.get()) {
     return syncer::SyncError(FROM_HERE,
                              syncer::SyncError::DATATYPE_ERROR,
                              "App List syncable service is not started.",
                              syncer::APP_LIST);
   }
-
+  model_observer_->set_processing_sync_changes(true);

[tapted, 2014/01/28 11:56:53]

It's a common pattern in the /ui/app_list code to temporarily remove the
observer, then add it back when finished when similar things like this occur -
should that be done here? (i.e. model_observer_.reset() here, and .reset(new ..)
down below)

[stevenjb, 2014/01/28 18:28:08]

Xiyuan made a similar comment, I explained why I didn't do that above, but could
be persuaded otherwise.

   DVLOG(1) << this << ": ProcessSyncChanges: " << change_list.size();
   for (syncer::SyncChangeList::const_iterator iter = change_list.begin();
        iter != change_list.end(); ++iter) {
     const SyncChange& change = *iter;
     DVLOG(2) << this << "  Change: "
              << change.sync_data().GetSpecifics().app_list().item_id()
              << " (" << change.change_type() << ")";
     if (change.change_type() == SyncChange::ACTION_ADD ||
         change.change_type() == SyncChange::ACTION_UPDATE) {
       ProcessSyncItemSpecifics(change.sync_data().GetSpecifics().app_list());
     } else if (change.change_type() == SyncChange::ACTION_DELETE) {
       DeleteSyncItemSpecifics(change.sync_data().GetSpecifics().app_list());
     } else {
       LOG(ERROR) << "Invalid sync change";
     }
   }
+  model_observer_->set_processing_sync_changes(false);
   return syncer::SyncError();
 }
 
 // AppListSyncableService private
 
 bool AppListSyncableService::ProcessSyncItemSpecifics(
     const sync_pb::AppListSpecifics& specifics) {
   const std::string& item_id = specifics.item_id();
   if (item_id.empty()) {
     LOG(ERROR) << "AppList item with empty ID";
     return false;
   }
   SyncItem* sync_item = FindSyncItem(item_id);
   if (sync_item) {
     // If an item of the same type exists, update it.
     if (sync_item->item_type == specifics.item_type()) {
       UpdateSyncItemFromSync(specifics, sync_item);
       ProcessExistingSyncItem(sync_item);
       DVLOG(2) << this << " <- SYNC UPDATE: " << sync_item->ToString();
       return false;
     }
     // Otherwise, one of the entries should be TYPE_REMOVE_DEFAULT_APP.
     if (sync_item->item_type !=
         sync_pb::AppListSpecifics::TYPE_REMOVE_DEFAULT_APP &&
         specifics.item_type() !=
         sync_pb::AppListSpecifics::TYPE_REMOVE_DEFAULT_APP) {
       LOG(ERROR) << "Synced item type: " << specifics.item_type()
                  << " != existing sync item type: " << sync_item->item_type
                  << " Deleting item from model!";
-      model_->item_list()->DeleteItem(item_id);
+      model_->DeleteItem(item_id);
     }
     DVLOG(2) << this << " - ProcessSyncItem: Delete existing entry: "
              << sync_item->ToString();
     delete sync_item;
     sync_items_.erase(item_id);
   }
 
   sync_item = CreateSyncItem(item_id, specifics.item_type());
   UpdateSyncItemFromSync(specifics, sync_item);
   ProcessNewSyncItem(sync_item);
@@ skipping 20 matching lines @@
       // TODO(stevenjb): Implement
       LOG(WARNING) << "TYPE_FOLDER not supported";
       return;
     }
     case sync_pb::AppListSpecifics::TYPE_URL: {
       // TODO(stevenjb): Implement
       LOG(WARNING) << "TYPE_URL not supported";
       return;
     }
   }
-  NOTREACHED() << "Unrecoginized sync item type: " << sync_item->ToString();
+  NOTREACHED() << "Unrecognized sync item type: " << sync_item->ToString();
 }
 
 void AppListSyncableService::ProcessExistingSyncItem(SyncItem* sync_item) {
   if (sync_item->item_type ==
       sync_pb::AppListSpecifics::TYPE_REMOVE_DEFAULT_APP) {
     return;
   }
   DVLOG(2) << "ProcessExistingSyncItem: " << sync_item->ToString();
-  AppListItem* app_item = model_->item_list()->FindItem(sync_item->item_id);
+  AppListItem* app_item = model_->FindItem(sync_item->item_id);
+  DVLOG(2) << " AppItem: " << app_item->ToDebugString();
   if (!app_item) {
     LOG(ERROR) << "Item not found in model: " << sync_item->ToString();
     return;
   }
   UpdateAppItemFromSyncItem(sync_item, app_item);
 }
 
 void AppListSyncableService::UpdateAppItemFromSyncItem(
     const AppListSyncableService::SyncItem* sync_item,
     AppListItem* app_item) {
   if (!app_item->position().Equals(sync_item->item_ordinal))
-    model_->item_list()->SetItemPosition(app_item, sync_item->item_ordinal);
+    model_->SetItemPosition(app_item, sync_item->item_ordinal);
 }
 
 bool AppListSyncableService::SyncStarted() {
   if (sync_processor_.get())
     return true;
   if (flare_.is_null()) {
     DVLOG(2) << this << ": SyncStarted: Flare.";
     flare_ = sync_start_util::GetFlareForSyncableService(profile_->GetPath());
     flare_.Run(syncer::APP_LIST);
   }
@@ skipping 46 matching lines @@
   DVLOG(2) << this << ": DeleteSyncItemSpecifics: " << item_id.substr(0, 8);
   SyncItemMap::iterator iter = sync_items_.find(item_id);
   if (iter == sync_items_.end())
     return;
   sync_pb::AppListSpecifics::AppListItemType item_type =
       iter->second->item_type;
   DVLOG(2) << this << " <- SYNC DELETE: " << iter->second->ToString();
   delete iter->second;
   sync_items_.erase(iter);
   if (item_type != sync_pb::AppListSpecifics::TYPE_REMOVE_DEFAULT_APP)
-    model_->item_list()->DeleteItem(item_id);
+    model_->DeleteItem(item_id);
 }
 
 std::string AppListSyncableService::SyncItem::ToString() const {
   std::string res = item_id.substr(0, 8);
   if (item_type == sync_pb::AppListSpecifics::TYPE_REMOVE_DEFAULT_APP) {
     res += " { RemoveDefault }";
   } else {
     res += " { " + item_name + " }";
     res += " [" + item_ordinal.ToDebugString() + "]";
+    if (!parent_id.empty())
+      res += " <" + parent_id.substr(0, 8) + ">";
   }
   return res;
 }
 
 }  // namespace app_list