Implement local storage for App List in case app sync is off.

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"
 #include "base/memory/ptr_util.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "build/build_config.h"
 #include "chrome/browser/apps/drive/drive_app_provider.h"
 #include "chrome/browser/extensions/extension_service.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/common/chrome_switches.h"
 #include "chrome/common/extensions/extension_constants.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/grit/generated_resources.h"
+#include "components/pref_registry/pref_registry_syncable.h"
 #include "components/sync/model/sync_change_processor.h"
 #include "components/sync/model/sync_data.h"
 #include "components/sync/model/sync_merge_result.h"
 #include "components/sync/protocol/sync.pb.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/uninstall_reason.h"
 #include "extensions/common/constants.h"
 #include "ui/app_list/app_list_folder_item.h"
 #include "ui/app_list/app_list_item.h"
@@ skipping 12 matching lines @@
 #endif
 
 using syncer::SyncChange;
 
 namespace app_list {
 
 namespace {
 
 const char kOemFolderId[] = "ddb1da55-d478-4243-8642-56d3041f0263";
 
+const char kNameKey[] = "name";
+const char kParentIdKey[] = "parent_id";
+const char kPositionKey[] = "position";
+const char kPinPositionKey[] = "pin_position";
+const char kTypeKey[] = "type";
+
 // Prefix for a sync id of a Drive app. Drive app ids are in a different
 // format and have to be used because a Drive app could have only an URL
 // without a matching Chrome app. To differentiate the Drive app id from
 // Chrome app ids, this prefix will be added to create the sync item id
 // for a Drive app item.
 const char kDriveAppSyncIdPrefix[] = "drive-app-";
 
 void UpdateSyncItemFromSync(const sync_pb::AppListSpecifics& specifics,
                             AppListSyncableService::SyncItem* item) {
   DCHECK_EQ(item->item_id, specifics.item_id());
@@ skipping 100 matching lines @@
 std::string GetDriveAppSyncId(const std::string& drive_app_id) {
   return kDriveAppSyncIdPrefix + drive_app_id;
 }
 
 std::string GetDriveAppIdFromSyncId(const std::string& sync_id) {
   if (!IsDriveAppSyncId(sync_id))
     return std::string();
   return sync_id.substr(strlen(kDriveAppSyncIdPrefix));
 }
 
+void RemoveSyncItemFromLocalStorage(Profile* profile,
+                                    const std::string& item_id) {
+  DictionaryPrefUpdate(profile->GetPrefs(), prefs::kAppListLocalState)->
+      Remove(item_id, nullptr);
+}
+
+void UpdateSyncItemInLocalStorage(
+    Profile* profile,
+    const AppListSyncableService::SyncItem* sync_item) {
+  DictionaryPrefUpdate pref_update(profile->GetPrefs(),
+                                   prefs::kAppListLocalState);
+  base::DictionaryValue* dict_item = nullptr;
+  if (!pref_update->GetDictionaryWithoutPathExpansion(sync_item->item_id,
+      &dict_item)) {
+    dict_item = new base::DictionaryValue();
+    pref_update->SetWithoutPathExpansion(sync_item->item_id, dict_item);
+  }
+
+  dict_item->SetString(kNameKey, sync_item->item_name);
+  dict_item->SetString(kParentIdKey, sync_item->parent_id);
+  dict_item->SetString(kPositionKey,sync_item->item_ordinal.IsValid() ?
+      sync_item->item_ordinal.ToInternalValue() : std::string());
+  dict_item->SetString(kPinPositionKey, sync_item->item_pin_ordinal.IsValid() ?
+      sync_item->item_pin_ordinal.ToInternalValue() : std::string());
+  dict_item->SetInteger(kTypeKey, static_cast<int>(sync_item->item_type));
+}
+
 }  // namespace
 
 // AppListSyncableService::SyncItem
 
 AppListSyncableService::SyncItem::SyncItem(
     const std::string& id,
     sync_pb::AppListSpecifics::AppListItemType type)
     : item_id(id),
       item_type(type) {
 }
@@ skipping 61 matching lines @@
   }
 
   AppListSyncableService* owner_;
   AppListItem* adding_item_;  // Unowned pointer to item being added.
 
   DISALLOW_COPY_AND_ASSIGN(ModelObserver);
 };
 
 // AppListSyncableService
 
+// static
+void AppListSyncableService::RegisterProfilePrefs(
+    user_prefs::PrefRegistrySyncable* registry) {
+  registry->RegisterDictionaryPref(prefs::kAppListLocalState);
+}
+
 AppListSyncableService::AppListSyncableService(
     Profile* profile,
     extensions::ExtensionSystem* extension_system)
     : profile_(profile),
       extension_system_(extension_system),
       model_(new AppListModel),
       initial_sync_data_processed_(false),
       first_app_list_sync_(true) {
   if (!extension_system) {
     LOG(ERROR) << "AppListSyncableService created with no ExtensionSystem";
     return;
   }
 
   oem_folder_name_ =
       l10n_util::GetStringUTF8(IDS_APP_LIST_OEM_DEFAULT_FOLDER_NAME);
+
+  InitFromLocalStorage();
 }
 
 AppListSyncableService::~AppListSyncableService() {
   // Remove observers.
   model_observer_.reset();
 }
 
+void AppListSyncableService::InitFromLocalStorage() {
+  if (switches::IsFolderUIEnabled())
+    model_->SetFoldersEnabled(true);

[stevenjb, 2016/10/20 17:04:46]

We will need to test this, but in theory with local pref storage we should be
able to always set folders enabled to true. Previously we disabled folders if
sync was enabled because we were not persisting folder state, but if this is
working correctly there is no reason to disable it.

[khmel, 2016/10/20 18:00:06]

Right, we can move it into CTOR and drop settings at other places.

+  // Restore initial state from local storage.
+  const base::DictionaryValue* local_items = profile_->GetPrefs()->
+      GetDictionary(prefs::kAppListLocalState);
+  DCHECK(local_items);
+
+  for (base::DictionaryValue::Iterator item(*local_items); !item.IsAtEnd();
+      item.Advance()) {
+    const base::DictionaryValue* dict_item;
+    if (!item.value().GetAsDictionary(&dict_item)) {
+      LOG(ERROR) << "Dictionary not found for " << item.key() + ".";
+      continue;
+    }
+
+    int type;
+    if (!dict_item->GetInteger(kTypeKey, &type)) {
+      LOG(ERROR) << "Item type is not set in local storage for " << item.key()
+                 << ".";
+      continue;
+    }
+
+    SyncItem* sync_item = CreateSyncItem(item.key(),
+        static_cast<sync_pb::AppListSpecifics::AppListItemType>(type));
+
+    dict_item->GetString(kNameKey, &sync_item->item_name);
+    dict_item->GetString(kParentIdKey, &sync_item->parent_id);
+    std::string position;
+    std::string pin_position;
+    dict_item->GetString(kPositionKey, &position);
+    dict_item->GetString(kPinPositionKey, &pin_position);
+    if (!position.empty())
+      sync_item->item_ordinal = syncer::StringOrdinal(position);
+    if (!pin_position.empty())
+      sync_item->item_pin_ordinal = syncer::StringOrdinal(pin_position);
+    ProcessNewSyncItem(sync_item);
+  }
+}
+
 void AppListSyncableService::BuildModel() {
   // TODO(calamity): make this a DCHECK after a dev channel release.
   CHECK(extension_system_->extension_service() &&
         extension_system_->extension_service()->is_ready());
   AppListControllerDelegate* controller = NULL;
   AppListService* service = AppListService::Get();
   if (service)
     controller = service->GetControllerDelegate();
   apps_builder_.reset(new ExtensionAppModelBuilder(controller));
 #if defined(OS_CHROMEOS)
@@ skipping 13 matching lines @@
     VLOG(1) << this << ": AppListSyncableService: InitializeWithProfile.";
     apps_builder_->InitializeWithProfile(profile_, model_.get());
 #if defined(OS_CHROMEOS)
     if (arc_apps_builder_.get())
       arc_apps_builder_->InitializeWithProfile(profile_, model_.get());
 #endif
   }
 
   if (app_list::switches::IsDriveAppsInAppListEnabled())
     drive_app_provider_.reset(new DriveAppProvider(profile_, this));
+
+  ResolveFolderPositions();
+
+  // Start observing app list model changes.
+  model_observer_.reset(new ModelObserver(this));

[stevenjb, 2016/10/20 17:04:46]

If we start observing the model here, don't we need to reset it in
MergeDataAndStartSyncing (right after the GetModel() call) to make sure we do
not process model events while building the model from sync data? Also these
should probably be added to the bottom of InitFromLocalStorage instead? (Which
may also need to reset the observer at the top of the function?

[khmel, 2016/10/20 18:00:06]

With local storage support yes, we also need to reset observer in
MergeDataAndStartSyncing. However I cannot do the same here, in InitLocalStorage
because it is called from CTOR and at this moment check at line 364 fails. I
considered moving InitLocalStorage to the BuildModel call but this is not
compatible with accessing to pin position, which can be made regardless if model
built or not.

[stevenjb, 2016/10/21 20:03:10]

Is there any harm in requiring the model to be build before accessing the pin
position, just to be consistent? 

It still seems strange to me to add the model observer here, since in
AppListSyncableService::MergeDataAndStartSyncing we call GetModel() (likely for
the first time), and then call model_observer_.reset() right after.

[khmel, 2016/10/24 17:28:39]

There is no harm to have model built before pin access. It seems that model is
pure optional part of this service and it is only required when app launcher is
created. Calling GetModel()from MergeDataAndStartSyncing is not needed. Probably
we may address this (making model pure on demand) if we want in separate CL.

Calling GetModel() during InitFromLocalStorage is inconvenient. When model is
building it may(actually does) modify sync service by creating sync items if
they don't exist. We track this changes and update local storage. This causes
race condition when building the model before initing from prefs overrides prefs
for certain entries with default values. Of cause it is possible to handle such
changes by adding some internal flag but this could complicate the logic. 

IMHO, settings observer on model building makes sense. Before we did not do this
due sync_processor was not set and we could do nothing. However now we also map
changes to prefs and handling external updates is essential for us.

I slightly refactored the logic to simplify everything and make it more
readable:
1. Build model when extension service is ready only
2. InitFromLocalStorage is now step of building model.
3. Extract logic to update folders, setting the observer and notifying model
updated to helper and call when required.

WDYT?

 }
 
 void AppListSyncableService::AddObserverAndStart(Observer* observer) {
   observer_list_.AddObserver(observer);
   SyncStarted();
 }
 
 void AppListSyncableService::RemoveObserver(Observer* observer) {
   observer_list_.RemoveObserver(observer);
 }
@@ skipping 116 matching lines @@
 }
 
 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);
+  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) {
   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;
 
   SyncItem* sync_item = FindSyncItem(app_item->id());
@@ skipping 32 matching lines @@
     LOG(ERROR) << "DeleteSyncItem: no sync item: " << item_id;
     return;
   }
   if (SyncStarted()) {
     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));
   }
+  RemoveSyncItemFromLocalStorage(profile_, item_id);
   sync_items_.erase(item_id);
 }
 
 void AppListSyncableService::UpdateSyncItem(AppListItem* app_item) {
   SyncItem* sync_item = FindSyncItem(app_item->id());
   if (!sync_item) {
     LOG(ERROR) << "UpdateItem: no sync item: " << app_item->id();
     return;
   }
   bool changed = UpdateSyncItemFromAppItem(app_item, sync_item);
   if (!changed) {
     DVLOG(2) << this << " - Update: SYNC NO CHANGE: " << sync_item->ToString();
     return;
   }
+  UpdateSyncItemInLocalStorage(profile_, sync_item);
   SendSyncChange(sync_item, SyncChange::ACTION_UPDATE);
 }
 
 void AppListSyncableService::RemoveItem(const std::string& id) {
   RemoveSyncItem(id);
   model_->DeleteItem(id);
   PruneEmptySyncFolders();
 }
 
 void AppListSyncableService::RemoveUninstalledItem(const std::string& id) {
@@ skipping 30 matching lines @@
     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.
     VLOG(2) << this << " -> SYNC UPDATE: REMOVE_DEFAULT: "
             << sync_item->item_id;
     sync_item->item_type = sync_pb::AppListSpecifics::TYPE_REMOVE_DEFAULT_APP;
+    UpdateSyncItemInLocalStorage(profile_, sync_item);
     SendSyncChange(sync_item, SyncChange::ACTION_UPDATE);
     return;
   }
 
   DeleteSyncItem(iter->first);
 }
 
 void AppListSyncableService::ResolveFolderPositions() {
   if (!app_list::switches::IsFolderUIEnabled())
     return;
@@ skipping 43 matching lines @@
     const syncer::SyncDataList& initial_sync_data,
     std::unique_ptr<syncer::SyncChangeProcessor> sync_processor,
     std::unique_ptr<syncer::SyncErrorFactory> error_handler) {
   DCHECK(!sync_processor_.get());
   DCHECK(sync_processor.get());
   DCHECK(error_handler.get());
 
   // Ensure the model is built.
   GetModel();
 
+  // Reset local state and recreate from sync info.
+  DictionaryPrefUpdate pref_update(profile_->GetPrefs(),
+                                   prefs::kAppListLocalState);
+  pref_update->Clear();
+
   sync_processor_ = std::move(sync_processor);
   sync_error_handler_ = std::move(error_handler);
   if (switches::IsFolderUIEnabled())
     model_->SetFoldersEnabled(true);
 
   syncer::SyncMergeResult result = syncer::SyncMergeResult(type);
   result.set_num_items_before_association(sync_items_.size());
   VLOG(1) << this << ": MergeDataAndStartSyncing: "
           << initial_sync_data.size();
 
@@ skipping 37 matching lines @@
   // 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);
     // 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)));
   }
   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.
   ResolveFolderPositions();
 
   // Start observing app list model changes.
   model_observer_.reset(new ModelObserver(this));
 
   NotifyObserversSyncUpdated();
 
   return result;
 }
 
 void AppListSyncableService::StopSyncing(syncer::ModelType type) {
   DCHECK_EQ(type, syncer::APP_LIST);
 
   sync_processor_.reset();
   sync_error_handler_.reset();
-  model_->SetFoldersEnabled(false);
 }
 
 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 (auto iter = sync_items_.begin(); iter != sync_items_.end(); ++iter) {
     VLOG(2) << this << " -> SYNC: " << iter->second->ToString();
@@ skipping 48 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);
+      UpdateSyncItemInLocalStorage(profile_, sync_item);
       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);
     }
     VLOG(2) << this << " - ProcessSyncItem: Delete existing entry: "
             << sync_item->ToString();
     sync_items_.erase(item_id);
   }
 
   sync_item = CreateSyncItem(item_id, specifics.item_type());
   UpdateSyncItemFromSync(specifics, sync_item);
   ProcessNewSyncItem(sync_item);
+  UpdateSyncItemInLocalStorage(profile_, sync_item);
   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
@@ skipping 131 matching lines @@
     LOG(ERROR) << "Delete AppList item with empty ID";
     return;
   }
   VLOG(2) << this << ": DeleteSyncItemSpecifics: " << item_id.substr(0, 8);
   auto iter = sync_items_.find(item_id);
   if (iter == sync_items_.end())
     return;
   sync_pb::AppListSpecifics::AppListItemType item_type =
       iter->second->item_type;
   VLOG(2) << this << " <- SYNC DELETE: " << iter->second->ToString();
+  RemoveSyncItemFromLocalStorage(profile_, item_id);
   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);
   } else if (item_type == sync_pb::AppListSpecifics::TYPE_REMOVE_DEFAULT_APP) {
     if (IsDriveAppSyncId(item_id) && drive_app_provider_) {
       drive_app_provider_->RemoveUninstalledDriveAppFromSync(
           GetDriveAppIdFromSyncId(item_id));
     }
   }
@@ skipping 86 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