Fix AppListSyncableService::ModelObserver actions

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/profiles/profile.h"
@@ skipping 117 matching lines @@
 }
 
 AppListSyncableService::SyncItem::~SyncItem() {
 }
 
 // AppListSyncableService::ModelObserver
 
 class AppListSyncableService::ModelObserver : public AppListModelObserver {
  public:
   explicit ModelObserver(AppListSyncableService* owner)
-      : owner_(owner) {
+      : owner_(owner),
+        adding_item_(NULL) {
     DVLOG(2) << owner_ << ": ModelObserver Added";
     owner_->model()->AddObserver(this);
   }
 
   virtual ~ModelObserver() {
     owner_->model()->RemoveObserver(this);
     DVLOG(2) << owner_ << ": ModelObserver Removed";
   }
 
  private:
   // AppListModelObserver
   virtual void OnAppListItemAdded(AppListItem* item) OVERRIDE {
-    DVLOG(2) << owner_ << " OnAppListItemAdded: " << item->ToDebugString();
+    DCHECK(!adding_item_);
+    adding_item_ = item;  // Ignore updates while adding an item.
+    VLOG(2) << owner_ << " OnAppListItemAdded: " << item->ToDebugString();
     owner_->AddOrUpdateFromSyncItem(item);
+    adding_item_ = NULL;
   }
 
   virtual void OnAppListItemWillBeDeleted(AppListItem* item) OVERRIDE {
-    DVLOG(2) << owner_ << " OnAppListItemDeleted: " << item->ToDebugString();
+    DCHECK(!adding_item_);
+    VLOG(2) << owner_ << " OnAppListItemDeleted: " << item->ToDebugString();
+    // Don't sync folder removal in case the folder still exists on another
+    // device (e.g. with device specific items in it). Empty folders will be
+    // deleted when the last item is removed (in PruneEmptySyncFolders()).
+    if (item->GetItemType() == AppListFolderItem::kItemType)
+      return;

[jennyz, 2014/04/09 22:00:32]

When users logs out and in again, the syncable service to prune the empty folder
after sync is done. How do we decide if the folder is empty? Will it know the
items not valid on this device but existing on other device?

[stevenjb, 2014/04/09 22:17:16]

AppListSyncableService has reliable information about folder state, so on
startup and any time we remove an item we call PruneEmptySyncFolders() and it
will remove sync folders. (My previous thinking that we only did that on startup
was incorrect, we do that when an item is removed also).

AppListModel does not know about items that are not installed, so it might
remove a folder prematurely, which is why we want to ignore this for folders.

     owner_->RemoveSyncItem(item->id());
   }
 
   virtual void OnAppListItemUpdated(AppListItem* item) OVERRIDE {
-    DVLOG(2) << owner_ << " OnAppListItemUpdated: " << item->ToDebugString();
+    if (adding_item_) {
+      // Adding an item may trigger update notifications which should be
+      // ignored.
+      DCHECK_EQ(adding_item_, item);
+      return;

[jennyz, 2014/04/09 21:31:08]

What if the update is not triggered by adding item, ie, a valid one, will sync
miss that update? 

[stevenjb, 2014/04/09 21:48:16]

This is all synchronous, so adding_item_ would be NULL for any update that is
not a side effect of adding the item.

[jennyz, 2014/04/09 22:00:32]

Sounds good. 

+    }
+    VLOG(2) << owner_ << " OnAppListItemUpdated: " << item->ToDebugString();
     owner_->UpdateSyncItem(item);
   }
 
   AppListSyncableService* owner_;
+  AppListItem* adding_item_;  // Unowned pointer to item being added.
 
   DISALLOW_COPY_AND_ASSIGN(ModelObserver);
 };
 
 // AppListSyncableService
 
 AppListSyncableService::AppListSyncableService(
     Profile* profile,
     extensions::ExtensionSystem* extension_system)
     : profile_(profile),
@@ skipping 121 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);
   UpdateSyncItemFromAppItem(app_item, sync_item);
   SendSyncChange(sync_item, SyncChange::ACTION_ADD);
   return sync_item;
 }
 
 void AppListSyncableService::AddOrUpdateFromSyncItem(AppListItem* app_item) {
   SyncItem* sync_item = FindSyncItem(app_item->id());
   if (sync_item) {
     UpdateAppItemFromSyncItem(sync_item, app_item);
@@ skipping 19 matching lines @@
 
   // Otherwise, we are adding the app as a non-default app (i.e. an app that
   // was installed by Default and removed is getting installed explicitly by
   // the user), so delete the REMOVE_DEFAULT_APP.
   DeleteSyncItem(sync_item);
   return false;
 }
 
 void AppListSyncableService::DeleteSyncItem(SyncItem* sync_item) {
   if (SyncStarted()) {
-    DVLOG(2) << this << " -> SYNC DELETE: " << sync_item->ToString();
+    VLOG(2) << this << " -> SYNC DELETE: " << sync_item->ToString();
     SyncChange sync_change(FROM_HERE, SyncChange::ACTION_DELETE,
                            GetSyncDataFromSyncItem(sync_item));
     sync_processor_->ProcessSyncChanges(
         FROM_HERE, syncer::SyncChangeList(1, sync_change));
   }
   std::string item_id = sync_item->item_id;
   delete sync_item;
   sync_items_.erase(item_id);
 }
 
@@ skipping 42 matching lines @@
   if (type == sync_pb::AppListSpecifics::TYPE_REMOVE_DEFAULT_APP) {
     // RemoveDefault item exists, just return.
     DVLOG(2) << this << " : RemoveDefault Item exists.";
     return;
   }
 
   if (type == sync_pb::AppListSpecifics::TYPE_APP &&
       AppIsDefault(extension_system_->extension_service(), id)) {
     // This is a Default app; update the entry to a REMOVE_DEFAULT entry. This
     // will overwrite any existing entry for the item.
-    DVLOG(2) << this << " -> SYNC UPDATE: REMOVE_DEFAULT: "
-             << sync_item->item_id;
+    VLOG(2) << this << " -> SYNC UPDATE: REMOVE_DEFAULT: "
+            << sync_item->item_id;
     sync_item->item_type = sync_pb::AppListSpecifics::TYPE_REMOVE_DEFAULT_APP;
     SendSyncChange(sync_item, SyncChange::ACTION_UPDATE);
     return;
   }
 
   DeleteSyncItem(sync_item);
 }
 
 void AppListSyncableService::ResolveFolderPositions(bool move_oem_to_end) {
   if (!app_list::switches::IsFolderUIEnabled())
@@ skipping 82 matching lines @@
   result.set_num_items_after_association(sync_items_.size());
   result.set_num_items_added(new_items);
   result.set_num_items_deleted(0);
   result.set_num_items_modified(updated_items);
 
   // Send unsynced items. Does not affect |result|.
   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();
+    VLOG(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);
 
   // Adding items may have created folders without setting their positions
   // since we haven't started observing the item list yet. Resolve those.
   // Also ensure the OEM folder is at the end if its position hasn't been set.
   bool move_oem_to_end = !oem_folder_is_synced;
   ResolveFolderPositions(move_oem_to_end);
@@ skipping 12 matching lines @@
 }
 
 syncer::SyncDataList AppListSyncableService::GetAllSyncData(
     syncer::ModelType type) const {
   DCHECK_EQ(syncer::APP_LIST, type);
 
   VLOG(1) << this << ": GetAllSyncData: " << sync_items_.size();
   syncer::SyncDataList list;
   for (SyncItemMap::const_iterator iter = sync_items_.begin();
        iter != sync_items_.end(); ++iter) {
-    DVLOG(2) << this << " -> SYNC: " << iter->second->ToString();
+    VLOG(2) << this << " -> SYNC: " << iter->second->ToString();
     list.push_back(GetSyncDataFromSyncItem(iter->second));
   }
   return list;
 }
 
 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);
   }
 
   // Don't observe the model while processing incoming sync changes.
   model_observer_.reset();
 
   VLOG(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() << ")";
+    VLOG(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";
     }
   }
 
@@ skipping 11 matching lines @@
   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();
+      VLOG(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_->DeleteItem(item_id);
     }
-    DVLOG(2) << this << " - ProcessSyncItem: Delete existing entry: "
-             << sync_item->ToString();
+    VLOG(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);
-  DVLOG(2) << this << " <- SYNC ADD: " << sync_item->ToString();
+  VLOG(2) << this << " <- SYNC ADD: " << sync_item->ToString();
   return true;
 }
 
 void AppListSyncableService::ProcessNewSyncItem(SyncItem* sync_item) {
   VLOG(2) << "ProcessNewSyncItem: " << sync_item->ToString();
   switch (sync_item->item_type) {
     case sync_pb::AppListSpecifics::TYPE_APP: {
       // New apps are added through ExtensionAppModelBuilder.
       // TODO(stevenjb): Determine how to handle app items in sync that
       // are not installed (e.g. default / OEM apps).
@@ skipping 39 matching lines @@
       !AppIsOem(app_item->id())) {
     DVLOG(2) << " Moving Item To Folder: " << sync_item->parent_id;
     model_->MoveItemToFolder(app_item, sync_item->parent_id);
   }
   UpdateAppItemFromSyncItem(sync_item, app_item);
 }
 
 void AppListSyncableService::UpdateAppItemFromSyncItem(
     const AppListSyncableService::SyncItem* sync_item,
     AppListItem* app_item) {
+  VLOG(2) << this << "UpdateAppItemFromSyncItem: " << sync_item->ToString();
   if (!app_item->position().Equals(sync_item->item_ordinal))
     model_->SetItemPosition(app_item, sync_item->item_ordinal);
   // Only update the item name if it is a Folder or the name is empty.
   if (sync_item->item_name != app_item->name() &&
       sync_item->item_id != kOemFolderId &&
       (app_item->GetItemType() == AppListFolderItem::kItemType ||
        app_item->name().empty())) {
     model_->SetItemName(app_item, sync_item->item_name);
   }
 }
@@ skipping 11 matching lines @@
 
 void AppListSyncableService::SendSyncChange(
     SyncItem* sync_item,
     SyncChange::SyncChangeType sync_change_type) {
   if (!SyncStarted()) {
     DVLOG(2) << this << " - SendSyncChange: SYNC NOT STARTED: "
              << sync_item->ToString();
     return;
   }
   if (sync_change_type == SyncChange::ACTION_ADD)
-    DVLOG(2) << this << " -> SYNC ADD: " << sync_item->ToString();
+    VLOG(2) << this << " -> SYNC ADD: " << sync_item->ToString();
   else
-    DVLOG(2) << this << " -> SYNC UPDATE: " << sync_item->ToString();
+    VLOG(2) << this << " -> SYNC UPDATE: " << sync_item->ToString();
   SyncChange sync_change(FROM_HERE, sync_change_type,
                          GetSyncDataFromSyncItem(sync_item));
   sync_processor_->ProcessSyncChanges(
       FROM_HERE, syncer::SyncChangeList(1, sync_change));
 }
 
 AppListSyncableService::SyncItem*
 AppListSyncableService::FindSyncItem(const std::string& item_id) {
   SyncItemMap::iterator iter = sync_items_.find(item_id);
   if (iter == sync_items_.end())
@@ skipping 17 matching lines @@
   if (item_id.empty()) {
     LOG(ERROR) << "Delete AppList item with empty ID";
     return;
   }
   VLOG(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();
+  VLOG(2) << this << " <- SYNC DELETE: " << iter->second->ToString();
   delete iter->second;
   sync_items_.erase(iter);
   // Only delete apps from the model. Folders will be deleted when all
   // children have been deleted.
   if (item_type == sync_pb::AppListSpecifics::TYPE_APP)
     model_->DeleteItem(item_id);
 }
 
 std::string AppListSyncableService::FindOrCreateOemFolder() {
   AppListFolderItem* oem_folder = model_->FindFolderItem(kOemFolderId);
@@ skipping 22 matching lines @@
   } else {
     res += " { " + item_name + " }";
     res += " [" + item_ordinal.ToDebugString() + "]";
     if (!parent_id.empty())
       res += " <" + parent_id.substr(0, 8) + ">";
   }
   return res;
 }
 
 }  // namespace app_list