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

chrome/browser/extensions/extension_service.h

Index: chrome/browser/extensions/extension_service.h
diff --git a/chrome/browser/extensions/extension_service.h b/chrome/browser/extensions/extension_service.h
index 68d2368976ec69ba73e0133bd28988a756f305c7..ffabbf4c07957e5ba0ca599b56b833710a7a6b1a 100644
--- a/chrome/browser/extensions/extension_service.h
+++ b/chrome/browser/extensions/extension_service.h
@@ -28,12 +28,15 @@
 #include "chrome/browser/extensions/extension_prefs.h"
 #include "chrome/browser/extensions/extension_permissions_api.h"
 #include "chrome/browser/extensions/extension_process_manager.h"
+#include "chrome/browser/extensions/extension_sync_data.h"
 #include "chrome/browser/extensions/extension_toolbar_model.h"
 #include "chrome/browser/extensions/extensions_quota_service.h"
 #include "chrome/browser/extensions/external_extension_provider_interface.h"
 #include "chrome/browser/extensions/pending_extension_manager.h"
 #include "chrome/browser/extensions/sandboxed_extension_unpacker.h"
 #include "chrome/browser/prefs/pref_change_registrar.h"
+#include "chrome/browser/sync/api/sync_change.h"
+#include "chrome/browser/sync/api/syncable_service.h"
 #include "chrome/common/extensions/extension.h"
 #include "content/browser/browser_thread.h"
 #include "content/common/notification_observer.h"
@@ -52,18 +55,19 @@ class ExtensionManagementEventRouter;
 class ExtensionPreferenceEventRouter;
 class ExtensionServiceBackend;
 class ExtensionSettings;
-struct ExtensionSyncData;
+class ExtensionSyncData;
 class ExtensionToolbarModel;
 class ExtensionUpdater;
 class ExtensionWebNavigationEventRouter;
 class GURL;
 class PendingExtensionManager;
 class Profile;
+class SyncData;
 class Version;
 
 // This is an interface class to encapsulate the dependencies that
 // various classes have on ExtensionService. This allows easy mocking.
-class ExtensionServiceInterface {
+class ExtensionServiceInterface : public SyncableService {
  public:
   // A function that returns true if the given extension should be
   // included and false if it should be filtered out.  Identical to
@@ -99,34 +103,6 @@ class ExtensionServiceInterface {
   // TODO(akalin): Remove this method (and others) once we refactor
   // themes sync to not use it directly.
   virtual void CheckForUpdatesSoon() = 0;
-
-  // Methods used by sync.
-  //
-  // TODO(akalin): We'll eventually need separate methods for app
-  // sync.  See http://crbug.com/58077 and http://crbug.com/61447.
-
-  // Get the sync data for |extension|.  If |extension| passes
-  // |filter|, fill in |extension_sync_data| and return true.
-  // Otherwise, return false.
-  //
-  // Ideally, we'd just have to pass in the extension ID, but the
-  // service may not know about the extension anymore (if it's
-  // unloaded).
-  virtual bool GetSyncData(const Extension& extension,
-                           ExtensionFilter filter,
-                           ExtensionSyncData* extension_sync_data) const = 0;
-
-  // Return a list of ExtensionSyncData objects for all extensions
-  // matching |filter|.
-  virtual std::vector<ExtensionSyncData> GetSyncDataList(
-      ExtensionFilter filter) const = 0;
-
-  // Take any actions required to make the local state of the
-  // extension match the state in |extension_sync_data| (including
-  // installing/uninstalling the extension).
-  virtual void ProcessSyncData(
-      const ExtensionSyncData& extension_sync_data,
-      ExtensionFilter filter) = 0;
 };
 
 // Manages installed and running Chromium extensions.
@@ -437,15 +413,15 @@ class ExtensionService
   virtual void CheckForUpdatesSoon() OVERRIDE;
 
   // Sync methods implementation.

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

Sync methods -> SyncableService

-  virtual bool GetSyncData(
-      const Extension& extension,
-      ExtensionFilter filter,
-      ExtensionSyncData* extension_sync_data) const OVERRIDE;
-  virtual std::vector<ExtensionSyncData> GetSyncDataList(
-      ExtensionFilter filter) const OVERRIDE;
-  virtual void ProcessSyncData(
-      const ExtensionSyncData& extension_sync_data,
-      ExtensionFilter filter) OVERRIDE;
+  virtual SyncError MergeDataAndStartSyncing(
+      syncable::ModelType type,
+      const SyncDataList& initial_sync_data,
+      SyncChangeProcessor* sync_processor) OVERRIDE;
+  virtual void StopSyncing(syncable::ModelType type) OVERRIDE;
+  virtual SyncDataList GetAllSyncData(syncable::ModelType type) const OVERRIDE;
+  virtual SyncError ProcessSyncChanges(
+      const tracked_objects::Location& from_here,
+      const SyncChangeList& change_list) OVERRIDE;
 
   void set_extensions_enabled(bool enabled) { extensions_enabled_ = enabled; }
   bool extensions_enabled() { return extensions_enabled_; }
@@ -561,6 +537,25 @@ class ExtensionService
 #endif
 
  private:
+  // Bundle of type (app or extension)-specific sync stuff.
+  struct SyncBundle {
+    static bool IsSyncableNone(const Extension& extension) { return false; }
+    SyncBundle() : filter(IsSyncableNone), sync_processor(NULL) { }

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

clang will probably complain about complex inline constructor, so you may want
to move them into the .cc file

Also, it'll probably complain about implicit destructor, so make it explicit and
move it to .cc file

+
+    bool HasExtensionId(const std::string& id) const {
+      return synced_extensions.find(id) != synced_extensions.end();
+    }
+
+    bool HasPendingExtensionId(const std::string& id) const {
+      return pending_sync_data.find(id) != pending_sync_data.end();
+    }
+
+    ExtensionFilter filter;
+    std::set<std::string> synced_extensions;
+    std::map<std::string, ExtensionSyncData> pending_sync_data;
+    SyncChangeProcessor* sync_processor;
+  };
+
   // Contains Extension data that can change during the life of the process,
   // but does not persist across restarts.
   struct ExtensionRuntimeData {
@@ -587,16 +582,36 @@ class ExtensionService
   };
   typedef std::list<NaClModuleInfo> NaClModuleInfoList;
 
-  // Gets the sync data for the given extension.
-  ExtensionSyncData GetSyncDataHelper(const Extension& extension) const;
+  // Notifies Sync (if needed) of a newly-installed or updated extension.

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

In the .cc file I see you also call this when an extension is enabled or
disabled or changes incognito setting, so perhaps update the comment here to
that effect. Also, a thought for you to consider - would a better name for the
function be SyncExtensionChangeIfNeeded? When I see the word "Update" I tend to
think of "an updated version of the extension was installed" but I might just be
weird since I've done a lot of the autoupdate stuff. 

+  void SyncExtensionUpdateIfNeeded(const Extension& extension);
+
+  // Get the appropriate SyncBundle, given some representation of Sync data.
+  SyncBundle* GetSyncBundleForExtension(const Extension& extension);
+  SyncBundle* GetSyncBundleForExtensionSyncData(
+      const ExtensionSyncData& extension_sync_data);
+  SyncBundle* GetSyncBundleForModelType(syncable::ModelType type);
+  const SyncBundle* GetSyncBundleForModelType(syncable::ModelType type) const;

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

maybe append Const to this fn name to avoid overloading (which is against google
style)

+
+  // Gets the ExtensionSyncData for all extensions.
+  std::vector<ExtensionSyncData> GetSyncDataList(
+      const SyncBundle& bundle) const;
+
+  // Gets the sync data for the given extension, assuming that the extension is
+  // syncable.
+  ExtensionSyncData GetSyncData(const Extension& extension) const;
 
   // Appends sync data objects for every extension in |extensions|
   // that passes |filter|.
   void GetSyncDataListHelper(
       const ExtensionList& extensions,
-      ExtensionFilter filter,
+      const SyncBundle& bundle,
       std::vector<ExtensionSyncData>* sync_data_list) const;
 
+  // Applies the change specified in an ExtensionSyncData to the current system.
+  void ProcessExtensionSyncData(

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

optional naming suggestion: consider just "ProcessSyncData"

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

Unfortunately, SyncData is a different class used by the sync engine--I want to
keep this separate for clarity.

+      const ExtensionSyncData& extension_sync_data,
+      SyncBundle& bundle);
+
   // Clear all persistent data that may have been stored by the extension.
   void ClearExtensionData(const GURL& extension_url);
 
@@ -778,6 +793,9 @@ class ExtensionService
 
   NaClModuleInfoList nacl_module_list_;
 
+  SyncBundle app_sync_bundle_;
+  SyncBundle extension_sync_bundle_;

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

A question for you to ponder - would it be easier to just have one SyncBundle
where we store both apps and extensions? It seems like there's some complexity
in several places in extension_service.cc around getting the right SyncBundle,
having an ExtensionFilter etc. that could maybe just be replaced with one or two
"if (extension->is_app())" statements. 

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

That's actually where I started, but I added the SyncBundle class when I started
implementing GetAllSyncData() and StopSyncing()--they end up having to parse out
synced_extensions and pending_sync_data to decide what to return or remove,
respectively.  It also doesn't save much, since we still have to find the
appropriate SyncChangeProcessor for a particular extension.

I do have a pile of helper methods to find the correct bundle, mostly because I
was repeating the same (or very similar) ten lines of code in many places.

The ExtensionFilters are artifacts of the original code, but they still seem
convenient for a few places--particularly the PendingExtensionManager,
GetAllSyncData(), and GetSyncBundleForExtension().  It wouldn't be too big of a
deal to pass an Extension::SyncType and compare to Extension::GetSyncType() in
those cases, though.

[asargent_no_longer_on_chrome, 2011/08/15 23:34:29]

Ok

+
   FRIEND_TEST_ALL_PREFIXES(ExtensionServiceTest,
                            InstallAppsWithUnlimtedStorage);
   FRIEND_TEST_ALL_PREFIXES(ExtensionServiceTest,