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

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 f7dd4d5f3854935f6c59c8c5ac09d99c802d0088..dd08cfdbdba83432f7d526ad4df2bb223b979b75 100644
--- a/chrome/browser/ui/app_list/app_list_syncable_service.cc
+++ b/chrome/browser/ui/app_list/app_list_syncable_service.cc
@@ -20,7 +20,9 @@
 #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"
@@ -53,6 +55,12 @@ 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
@@ -173,6 +181,33 @@ std::string GetDriveAppIdFromSyncId(const std::string& sync_id) {
   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
@@ -254,6 +289,12 @@ class AppListSyncableService::ModelObserver : public AppListModelObserver {
 
 // AppListSyncableService
 
+// static
+void AppListSyncableService::RegisterProfilePrefs(
+    user_prefs::PrefRegistrySyncable* registry) {
+  registry->RegisterDictionaryPref(prefs::kAppListLocalState);
+}
+
 AppListSyncableService::AppListSyncableService(
     Profile* profile,
     extensions::ExtensionSystem* extension_system)
@@ -269,6 +310,8 @@ AppListSyncableService::AppListSyncableService(
 
   oem_folder_name_ =
       l10n_util::GetStringUTF8(IDS_APP_LIST_OEM_DEFAULT_FOLDER_NAME);
+
+  InitFromLocalStorage();
 }
 
 AppListSyncableService::~AppListSyncableService() {
@@ -276,6 +319,46 @@ AppListSyncableService::~AppListSyncableService() {
   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() &&
@@ -309,6 +392,11 @@ void AppListSyncableService::BuildModel() {
 
   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) {
@@ -445,6 +533,7 @@ AppListSyncableService::CreateSyncItemFromAppItem(AppListItem* app_item) {
   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;
 }
@@ -470,6 +559,7 @@ void AppListSyncableService::SetPinPosition(
   }
 
   sync_item->item_pin_ordinal = item_pin_ordinal;
+  UpdateSyncItemInLocalStorage(profile_, sync_item);
   SendSyncChange(sync_item, sync_change_type);
 }
 
@@ -522,6 +612,7 @@ void AppListSyncableService::DeleteSyncItem(const std::string& item_id) {
     sync_processor_->ProcessSyncChanges(
         FROM_HERE, syncer::SyncChangeList(1, sync_change));
   }
+  RemoveSyncItemFromLocalStorage(profile_, item_id);
   sync_items_.erase(item_id);
 }
 
@@ -536,6 +627,7 @@ void AppListSyncableService::UpdateSyncItem(AppListItem* app_item) {
     DVLOG(2) << this << " - Update: SYNC NO CHANGE: " << sync_item->ToString();
     return;
   }
+  UpdateSyncItemInLocalStorage(profile_, sync_item);
   SendSyncChange(sync_item, SyncChange::ACTION_UPDATE);
 }
 
@@ -586,6 +678,7 @@ void AppListSyncableService::RemoveSyncItem(const std::string& id) {
     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;
   }
@@ -649,6 +742,11 @@ syncer::SyncMergeResult AppListSyncableService::MergeDataAndStartSyncing(
   // 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())
@@ -706,6 +804,7 @@ syncer::SyncMergeResult AppListSyncableService::MergeDataAndStartSyncing(
     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)));
   }
@@ -728,7 +827,6 @@ void AppListSyncableService::StopSyncing(syncer::ModelType type) {
 
   sync_processor_.reset();
   sync_error_handler_.reset();
-  model_->SetFoldersEnabled(false);
 }
 
 syncer::SyncDataList AppListSyncableService::GetAllSyncData(
@@ -797,6 +895,7 @@ bool AppListSyncableService::ProcessSyncItemSpecifics(
     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;
     }
@@ -818,6 +917,7 @@ bool AppListSyncableService::ProcessSyncItemSpecifics(
   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;
 }
@@ -969,7 +1069,9 @@ void AppListSyncableService::DeleteSyncItemSpecifics(
   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) {