Mirror app list hierarchy data in profile prefs.

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 f4d38d99435653204034badc8b1bda7df44e83f4..58b532bd935eea17f7a22ecdf2861944dc3dafac 100644
--- a/chrome/browser/ui/app_list/app_list_syncable_service.cc
+++ b/chrome/browser/ui/app_list/app_list_syncable_service.cc
@@ -9,6 +9,7 @@
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/app_list/app_list_prefs.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"
@@ -203,6 +204,54 @@ class AppListSyncableService::ModelObserver : public AppListModelObserver {
   DISALLOW_COPY_AND_ASSIGN(ModelObserver);
 };
 
+// AppListSyncableService::PrefUpdater
+
+class AppListSyncableService::PrefUpdater : public AppListModelObserver {
+ public:
+  explicit PrefUpdater(AppListPrefs* app_list_prefs, AppListModel* model)
+      : app_list_prefs_(app_list_prefs), model_(model) {

[stevenjb, 2014/09/26 16:22:16]

one line per arg

[Matt Giuca, 2014/09/30 02:49:47]

I think this is fine, as long as they are all on one line (ultimately, do
whatever ClangFormat does).

[calamity, 2014/09/30 05:40:08]

Yeah, this is ClangFormat's doing.

[stevenjb, 2014/09/30 18:16:23]

Hm, we should probably update the style guide then, but yes, we bow to
ClangFormat.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Constructor_I...

+    model_->AddObserver(this);
+  }
+
+  virtual ~PrefUpdater() { model_->RemoveObserver(this); }
+
+ private:
+  void UpdatePrefsFromAppListItem(AppListItem* item) {
+    // Write synced data to local pref.
+    AppListPrefs::AppListInfo info;
+    if (item->GetItemType() == AppListFolderItem::kItemType)
+      info.item_type = AppListPrefs::AppListInfo::FOLDER_ITEM;
+    else if (item->GetItemType() == ExtensionAppItem::kItemType)
+      info.item_type = AppListPrefs::AppListInfo::APP_ITEM;
+    else
+      NOTREACHED();
+
+    info.folder_id = item->folder_id();
+    info.position = item->position();
+    info.name = item->name();
+
+    app_list_prefs_->SetAppListInfo(item->id(), info);
+  }
+
+  // Overridden from AppListModelObserver:
+  void OnAppListItemAdded(AppListItem* item) override {
+    UpdatePrefsFromAppListItem(item);
+  }
+
+  void OnAppListItemWillBeDeleted(AppListItem* item) override {
+    app_list_prefs_->DeleteAppListInfo(item->id());
+  }
+
+  void OnAppListItemUpdated(AppListItem* item) override {
+    UpdatePrefsFromAppListItem(item);
+  }
+
+  AppListPrefs* app_list_prefs_;
+  AppListModel* model_;
+
+  DISALLOW_COPY_AND_ASSIGN(PrefUpdater);
+};

[stevenjb, 2014/09/26 16:22:16]

This appears to be entirely independent of AppListSyncableService, I think it
should be implemented in a separate file.

[Matt Giuca, 2014/09/30 02:49:47]

It's simple and could be considered an implementation detail of
AppListSyncableService; I think it's fine to leave it here.

[calamity, 2014/09/30 05:40:08]

I also think of it as a utility class that allows the AppListSyncableService to
fulfill its responsibility to update the AppListPrefs. I could have shoved it
into the existing ModelObserver but I think the separation of responsibilities
of each observer is nicer.

[stevenjb, 2014/09/30 18:16:23]

The reason I don't especially like it here is that it is not an implementation
detail of AppListSyncableService, there are no co-dependencies here, other than
that AppListSyncableService is a convenient place to own PrefUpdater (which I
think is fine). Having it here, especially as a member class, implies that there
are dependencies between the classes when if fact there are not. PrefUpdater
relies solely on AppListModel (as it should).

I think there might be some complexity when we implement conflict resolution
between AppListSyncableService and AppListPrefs. I think that having PrefUpdater
separated from AppListSyncableService might help lead to a cleaner / more
maintainable result.

[calamity, 2014/10/01 08:46:14]

Done.

FYI, I think our conflict resolution plan is to always have sync overwrite local
data. This means there is a brief window where local changes will be clobbered
but I think that's okay for now. If we really want to fix this, we can cache
local changes until the sync service is ready and then flush them.

Therefore the prefs should only ever write to the model when the
AppListSyncableService is first created, before MergeDataAndStartSyncing is
called.

When signing in for the first time without any prior sync data, the syncable
service will dump all the local prefs into sync.

[stevenjb, 2014/10/01 16:29:33]

There are a couple of things that concern me off the top of my head. We probably
shouldn't discuss them here, but just to mention them:

1. A Windows user with sync disabled installs and organizes a bunch of apps.
They also have a bunch of apps on a Chromebook. When they enable sync on
Windows, do we clobber all of the local data? What about apps that were not
installed on the Chromebook?

2."When signing in for the first time without any prior sync data" isn't
entirely straightforward. We don't know whether or not we have any sync data
until MergeDataAndStartSyncing gets called. At that point, the initial data set
might have positions for some apps that are in AppListPrefs but not others.

*Hopefully* we will be able to resolve this in a straightforward manner that is
satisfactory, but if there is some complexity there, I would like to keep it out
of this already large and complicated class/file, so thanks.


I am also hopeful that a straightforward approach will only have minor edge
cases that we do not need to be concerned with, but if we do run into any
complexity I think it will be helpful to have these classes separated, so
thanks.

+
 // AppListSyncableService
 
 AppListSyncableService::AppListSyncableService(
@@ -264,6 +313,9 @@ void AppListSyncableService::BuildModel() {
     apps_builder_->InitializeWithProfile(profile_, model_.get());
   }
 
+  pref_updater_.reset(
+      new PrefUpdater(AppListPrefs::Get(profile_), model_.get()));
+
   if (app_list::switches::IsDriveAppsInAppListEnabled())
     drive_app_provider_.reset(new DriveAppProvider(profile_));
 }