Apps/Extensions Sync refactoring -- delete most of the old glue, implement new sync API.

chrome/browser/extensions/extension_service.cc

Index: chrome/browser/extensions/extension_service.cc
diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index b49f5bd16bee31a708374fd37a6721e01c409bb2..37eaf7f3697f4e007f3d0c4c98a6ebbd8250899c 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -53,6 +53,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search_engines/template_url_service.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
+#include "chrome/browser/sync/api/sync_change.h"
 #include "chrome/browser/themes/theme_service.h"
 #include "chrome/browser/themes/theme_service_factory.h"
 #include "chrome/browser/ui/webui/chrome_url_data_manager.h"
@@ -151,6 +152,14 @@ static void ForceShutdownPlugin(const FilePath& plugin_path) {
     plugin->ForceShutdown();
 }
 
+static bool IsSyncableExtension(const Extension& extension) {
+  return extension.GetSyncType() == Extension::SYNC_TYPE_EXTENSION;
+}
+
+static bool IsSyncableApp(const Extension& extension) {
+  return extension.GetSyncType() == Extension::SYNC_TYPE_APP;
+}
+
 // Manages an ExtensionInstallUI for a particular extension.
 class SimpleExtensionLoadPrompt : public ExtensionInstallUI::Delegate {
  public:
@@ -517,8 +526,9 @@ const Extension* ExtensionService::GetInstalledAppForRenderer(
 
 // static
 // This function is used to implement the command-line switch
-// --uninstall-extension.  The LOG statements within this function are used to
-// inform the user if the uninstall cannot be done.
+// --uninstall-extension, and to uninstall an extension via sync.  The LOG
+// statements within this function are used to inform the user if the uninstall
+// cannot be done.
 bool ExtensionService::UninstallExtensionHelper(
     ExtensionService* extensions_service,
     const std::string& extension_id) {
@@ -855,6 +865,18 @@ bool ExtensionService::UninstallExtension(
     return false;
   }
 
+  // Extract the data we need for sync now, but don't actually sync until we've
+  // completed the uninstallation.
+  SyncBundle* sync_bundle = GetSyncBundleForExtension(*extension);
+
+  SyncChange sync_change;
+  if (sync_bundle) {
+    ExtensionSyncData extension_sync_data(*extension,
+                                          IsExtensionEnabled(extension_id),
+                                          IsIncognitoEnabled(extension_id));
+    sync_change = extension_sync_data.GetSyncChange(SyncChange::ACTION_DELETE);
+  }
+
   UninstalledExtensionInfo uninstalled_extension_info(*extension);
 
   UMA_HISTOGRAM_ENUMERATION("Extensions.UninstallType",
@@ -894,6 +916,14 @@ bool ExtensionService::UninstallExtension(
       Source<Profile>(profile_),
       Details<UninstalledExtensionInfo>(&uninstalled_extension_info));
 
+  if (sync_bundle && sync_bundle->HasExtensionId(extension_id)) {
+    SyncChangeList sync_change_list(1);
+    sync_change_list[0] = sync_change;
+    sync_bundle->sync_processor->ProcessSyncChanges(
+        FROM_HERE, sync_change_list);
+    sync_bundle->synced_extensions.erase(extension_id);
+  }
+
   return true;
 }
 
@@ -952,6 +982,8 @@ void ExtensionService::EnableExtension(const std::string& extension_id) {
   extension_prefs_->SetBrowserActionVisibility(extension, true);
 
   NotifyExtensionLoaded(extension);
+
+  SyncExtensionUpdateIfNeeded(*extension);
 }
 
 void ExtensionService::DisableExtension(const std::string& extension_id) {
@@ -988,6 +1020,8 @@ void ExtensionService::DisableExtension(const std::string& extension_id) {
   }
 
   NotifyExtensionUnloaded(extension, UnloadedExtensionInfo::DISABLE);
+
+  SyncExtensionUpdateIfNeeded(*extension);
 }
 
 void ExtensionService::GrantPermissions(const Extension* extension) {
@@ -1618,60 +1652,204 @@ void ExtensionService::CheckForUpdatesSoon() {
   }
 }
 
-ExtensionSyncData ExtensionService::GetSyncDataHelper(
-    const Extension& extension) const {
-  const std::string& id = extension.id();
-  ExtensionSyncData data;
-  data.id = id;
-  data.uninstalled = false;
-  data.enabled = IsExtensionEnabled(id);
-  data.incognito_enabled = IsIncognitoEnabled(id);
-  data.version = *extension.version();
-  data.update_url = extension.update_url();
-  data.name = extension.name();
-  return data;
+void ExtensionService::SyncExtensionUpdateIfNeeded(const Extension& extension) {
+  SyncBundle* sync_bundle = GetSyncBundleForExtension(extension);
+  if (sync_bundle) {
+    SyncChangeList sync_change_list(1);
+    ExtensionSyncData extension_sync_data(extension,
+                                          IsExtensionEnabled(extension.id()),
+                                          IsIncognitoEnabled(extension.id()));
+
+    sync_change_list[0] = extension_sync_data.GetSyncChange(
+        sync_bundle->HasExtensionId(extension.id()) ?
+            SyncChange::ACTION_UPDATE : SyncChange::ACTION_ADD);
+    sync_bundle->sync_processor->ProcessSyncChanges(
+        FROM_HERE, sync_change_list);
+    sync_bundle->synced_extensions.insert(extension.id());
+    sync_bundle->pending_sync_data.erase(extension.id());
+  }
+}
+
+ExtensionService::SyncBundle* ExtensionService::GetSyncBundleForExtension(
+    const Extension& extension) {
+  if (app_sync_bundle_.filter(extension))
+    return &app_sync_bundle_;
+  else if (extension_sync_bundle_.filter(extension))
+    return &extension_sync_bundle_;
+  else
+    return NULL;
 }
 
-bool ExtensionService::GetSyncData(
-    const Extension& extension,
-    ExtensionFilter filter,
-    ExtensionSyncData* extension_sync_data) const {
-  if (!(*filter)(extension)) {
-    return false;
+ExtensionService::SyncBundle*
+    ExtensionService::GetSyncBundleForExtensionSyncData(
+    const ExtensionSyncData& extension_sync_data) {
+  switch (extension_sync_data.type()) {
+    case Extension::SYNC_TYPE_APP:
+      return &app_sync_bundle_;
+    case Extension::SYNC_TYPE_EXTENSION:
+      return &extension_sync_bundle_;
+    default:
+      NOTREACHED();
+      return NULL;
+  }
+}
+
+const ExtensionService::SyncBundle* ExtensionService::GetSyncBundleForModelType(
+    syncable::ModelType type) const {
+  switch (type) {
+    case syncable::APPS:
+      return &app_sync_bundle_;
+    case syncable::EXTENSIONS:
+      return &extension_sync_bundle_;
+    default:
+      NOTREACHED();
+      return NULL;
+  }
+}
+
+ExtensionService::SyncBundle* ExtensionService::GetSyncBundleForModelType(
+    syncable::ModelType type) {
+  switch (type) {
+    case syncable::APPS:
+      return &app_sync_bundle_;
+    case syncable::EXTENSIONS:
+      return &extension_sync_bundle_;
+    default:
+      NOTREACHED();
+      return NULL;

[asargent_no_longer_on_chrome, 2011/08/12 20:50:39]

nit: Looks like these two have the same body. Maybe use a macro or templates or
something to eliminate the duplication?

   }
-  *extension_sync_data = GetSyncDataHelper(extension);
-  return true;
+}
+
+SyncError ExtensionService::MergeDataAndStartSyncing(
+    syncable::ModelType type,
+    const SyncDataList& initial_sync_data,
+    SyncChangeProcessor* sync_processor) {
+
+  SyncBundle* bundle = NULL;
+
+  switch (type) {
+    case syncable::EXTENSIONS:
+      bundle = &extension_sync_bundle_;
+      bundle->filter = IsSyncableExtension;
+      break;
+
+    case syncable::APPS:
+      bundle = &app_sync_bundle_;
+      bundle->filter = IsSyncableApp;
+      break;
+
+    default:
+      DLOG(ERROR) << "Got " << type << " ModelType";
+      NOTREACHED();

[akalin, 2011/08/12 03:35:02]

may as well just do NOTREACHED() << "Got " ...;
Also return a suitable SyncError() here

[asargent_no_longer_on_chrome, 2011/08/12 20:50:39]

consider LOG(FATAL) 

+  }
+
+  bundle->sync_processor = sync_processor;

[asargent_no_longer_on_chrome, 2011/08/12 20:50:39]

consider adding a CHECK(sync_processor) right above this

+
+  for (SyncDataList::const_iterator i = initial_sync_data.begin();
+       i != initial_sync_data.end();
+       ++i) {
+    ExtensionSyncData extension_sync_data = ExtensionSyncData(*i);
+    bundle->synced_extensions.insert(extension_sync_data.id());
+    ProcessExtensionSyncData(extension_sync_data, *bundle);
+  }
+
+  SyncDataList sync_data_list = GetAllSyncData(type);
+  SyncChangeList sync_change_list;
+  for (SyncDataList::const_iterator i = sync_data_list.begin();
+       i != sync_data_list.end();
+       ++i) {
+    if (bundle->HasExtensionId(i->GetTag()))
+      sync_change_list.push_back(SyncChange(SyncChange::ACTION_UPDATE, *i));

[asargent_no_longer_on_chrome, 2011/08/12 20:50:39]

I was a bit surprised not to see you check here whether the local data about
this extension differs from what sync has, and if not then avoid adding an entry
to sync_change_list. Does the sync system skip pushing unnecessary changes up to
the server for you?

[Ben Olmstead, 2011/08/15 20:20:12]

According to Fred, yes it does.  I considered the case, but did not want to add
more code if the sync engine already handled it.

+    else
+      sync_change_list.push_back(SyncChange(SyncChange::ACTION_ADD, *i));
+  }
+  bundle->sync_processor->ProcessSyncChanges(FROM_HERE, sync_change_list);
+
+  return SyncError();
+}
+
+void ExtensionService::StopSyncing(syncable::ModelType type) {
+  SyncBundle* bundle = GetSyncBundleForModelType(type);
+  DCHECK(bundle);

[akalin, 2011/08/12 03:35:02]

change to CHECK, since you're dereferencing in next line anyway

+  // This is the simplest way to clear out the bundle.
+  *bundle = SyncBundle();
+}
+
+SyncDataList ExtensionService::GetAllSyncData(syncable::ModelType type) const {
+  const SyncBundle* bundle = GetSyncBundleForModelType(type);
+  DCHECK(bundle);

[akalin, 2011/08/12 03:35:02]

CHECK

+  std::vector<ExtensionSyncData> extension_sync_data = GetSyncDataList(*bundle);
+  SyncDataList rv(extension_sync_data.size());

[asargent_no_longer_on_chrome, 2011/08/12 20:50:39]

naming nit: "rv" is too abbreviated - maybe use "result" or something like that.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#General_Naming...

+  for (int i = 0; i < static_cast<int>(extension_sync_data.size()); ++i) {

[akalin, 2011/08/12 03:35:02]

may as well use iterators here, too

+    rv[i] = extension_sync_data[i].GetSyncData();
+  }
+  return rv;
+}
+
+SyncError ExtensionService::ProcessSyncChanges(
+    const tracked_objects::Location& from_here,
+    const SyncChangeList& change_list) {
+  for (SyncChangeList::const_iterator i = change_list.begin();
+      i != change_list.end();
+      ++i) {
+    ExtensionSyncData extension_sync_data = ExtensionSyncData(*i);
+    SyncBundle* bundle = GetSyncBundleForExtensionSyncData(extension_sync_data);
+    DCHECK(bundle);

[akalin, 2011/08/12 03:35:02]

CHECK

+
+    if (extension_sync_data.uninstalled())
+      bundle->synced_extensions.erase(extension_sync_data.id());
+    else
+      bundle->synced_extensions.insert(extension_sync_data.id());
+    ProcessExtensionSyncData(extension_sync_data, *bundle);
+  }
+
+  return SyncError();
 }
 
 void ExtensionService::GetSyncDataListHelper(
     const ExtensionList& extensions,
-    ExtensionFilter filter,
+    const SyncBundle& bundle,
     std::vector<ExtensionSyncData>* sync_data_list) const {
   for (ExtensionList::const_iterator it = extensions.begin();
        it != extensions.end(); ++it) {
     const Extension& extension = **it;
-    if ((*filter)(extension)) {
-      sync_data_list->push_back(GetSyncDataHelper(extension));
+    if (bundle.filter(extension) &&
+        // If we have pending extension data for this extension, then this
+        // version is out of date.  We'll sync back the version we got from
+        // sync.
+        !bundle.HasPendingExtensionId(extension.id())) {
+      sync_data_list->push_back(
+          ExtensionSyncData(extension,
+                            IsExtensionEnabled(extension.id()),
+                            IsIncognitoEnabled(extension.id())));
     }
   }
 }
 
 std::vector<ExtensionSyncData> ExtensionService::GetSyncDataList(
-    ExtensionFilter filter) const {
-  std::vector<ExtensionSyncData> sync_data_list;
-  GetSyncDataListHelper(extensions_, filter, &sync_data_list);
-  GetSyncDataListHelper(disabled_extensions_, filter, &sync_data_list);
-  GetSyncDataListHelper(terminated_extensions_, filter, &sync_data_list);
-  return sync_data_list;
+    const SyncBundle& bundle) const {
+  std::vector<ExtensionSyncData> extension_sync_list;
+  GetSyncDataListHelper(extensions_, bundle, &extension_sync_list);
+  GetSyncDataListHelper(disabled_extensions_, bundle, &extension_sync_list);
+  GetSyncDataListHelper(terminated_extensions_, bundle, &extension_sync_list);
+
+  for (std::map<std::string, ExtensionSyncData>::const_iterator i =
+           bundle.pending_sync_data.begin();
+       i != bundle.pending_sync_data.end();
+       ++i) {
+    extension_sync_list.push_back(i->second);
+  }
+
+  return extension_sync_list;
 }
 
-void ExtensionService::ProcessSyncData(
+void ExtensionService::ProcessExtensionSyncData(
     const ExtensionSyncData& extension_sync_data,
-    ExtensionFilter filter) {
-  const std::string& id = extension_sync_data.id;
+    SyncBundle& bundle) {
+  const std::string& id = extension_sync_data.id();
 
   // Handle uninstalls first.
-  if (extension_sync_data.uninstalled) {
+  if (extension_sync_data.uninstalled()) {
     std::string error;
     if (!UninstallExtensionHelper(this, id)) {
       LOG(WARNING) << "Could not uninstall extension " << id
@@ -1681,38 +1859,38 @@ void ExtensionService::ProcessSyncData(
   }
 
   // Set user settings.
-  if (extension_sync_data.enabled) {
+  if (extension_sync_data.enabled()) {
     EnableExtension(id);
   } else {
     DisableExtension(id);
   }
-  SetIsIncognitoEnabled(id, extension_sync_data.incognito_enabled);
+  SetIsIncognitoEnabled(id, extension_sync_data.incognito_enabled());
 
   const Extension* extension = GetInstalledExtension(id);
   if (extension) {
     // If the extension is already installed, check if it's outdated.
-    int result = extension->version()->CompareTo(extension_sync_data.version);
+    int result = extension->version()->CompareTo(extension_sync_data.version());
     if (result < 0) {
       // Extension is outdated.
+      bundle.pending_sync_data[extension_sync_data.id()] = extension_sync_data;
       CheckForUpdatesSoon();
-    } else if (result > 0) {
-      // Sync version is outdated.  Do nothing for now, as sync code
-      // in other places will eventually update the sync data.
-      //
-      // TODO(akalin): Move that code here.
     }
   } else {
     // TODO(akalin): Replace silent update with a list of enabled
     // permissions.
+    //
+    // TODO(bolms): Install disabled if it came in from sync disabled.

[akalin, 2011/08/12 03:35:02]

i believe the DisableExtension(id) call above should do exactly that already

[Ben Olmstead, 2011/08/15 20:20:12]

You are correct--just verified.

     const bool kInstallSilently = true;
     if (!pending_extension_manager()->AddFromSync(
             id,
-            extension_sync_data.update_url,
-            filter,
+            extension_sync_data.update_url(),
+            bundle.filter,
             kInstallSilently)) {
       LOG(WARNING) << "Could not add pending extension for " << id;

[akalin, 2011/08/12 03:35:02]

From the comments re. when AddFromSync returns false, I think we can conclude
the extension is already pending, so maybe returning is the wrong thing here.

[Ben Olmstead, 2011/08/15 20:20:12]

Looking at the code, it looks like this can only happen in fairly strange cases.
 The most likely scenario is a new extension coming in both through enterprise
policy and sync on Chrome startup.  Thinking through the cases, it does look
like it should get added to pending_sync_data, although it will stay there
forever.

       return;
     }
+    // Track pending extensions so that we can return them in GetAllSyncData().
+    bundle.pending_sync_data[extension_sync_data.id()] = extension_sync_data;
     CheckForUpdatesSoon();
   }
 }
@@ -1754,6 +1932,9 @@ void ExtensionService::SetIsIncognitoEnabled(
     NotifyExtensionUnloaded(enabled_extension, UnloadedExtensionInfo::DISABLE);
     NotifyExtensionLoaded(enabled_extension);
   }
+
+  if (extension)
+    SyncExtensionUpdateIfNeeded(*extension);
 }
 
 bool ExtensionService::CanCrossIncognito(const Extension* extension) {
@@ -2023,10 +2204,12 @@ void ExtensionService::AddExtension(const Extension* extension) {
         chrome::NOTIFICATION_EXTENSION_UPDATE_DISABLED,
         Source<Profile>(profile_),
         Details<const Extension>(extension));
+    SyncExtensionUpdateIfNeeded(*extension);
     return;
   }
 
   extensions_.push_back(scoped_extension);
+  SyncExtensionUpdateIfNeeded(*extension);
   NotifyExtensionLoaded(extension);
 }