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

chrome/browser/extensions/extension_service.h

 // Copyright (c) 2011 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.
 
 #ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_SERVICE_H_
 #define CHROME_BROWSER_EXTENSIONS_EXTENSION_SERVICE_H_
 #pragma once
 
 #include <map>
 #include <set>
@@ skipping 10 matching lines @@
 #include "base/task.h"
 #include "base/time.h"
 #include "base/tuple.h"
 #include "chrome/browser/extensions/apps_promo.h"
 #include "chrome/browser/extensions/extension_app_api.h"
 #include "chrome/browser/extensions/extension_icon_manager.h"
 #include "chrome/browser/extensions/extension_menu_manager.h"
 #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"
 #include "content/common/notification_registrar.h"
 #include "content/common/property_bag.h"
 
 class CrxInstaller;
 class ExtensionBookmarkEventRouter;
 class ExtensionBrowserEventRouter;
 class ExtensionContentSettingsStore;
 class ExtensionCookiesEventRouter;
 class ExtensionFileBrowserEventRouter;
 class ExtensionHistoryEventRouter;
 class ExtensionInstallUI;
 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
   // PendingExtensionInfo::ShouldAllowInstallPredicate.
   typedef bool (*ExtensionFilter)(const Extension&);
 
   virtual ~ExtensionServiceInterface() {}
   virtual const ExtensionList* extensions() const = 0;
   virtual PendingExtensionManager* pending_extension_manager() = 0;
 
@@ skipping 15 matching lines @@
 
   virtual void UpdateExtensionBlacklist(
     const std::vector<std::string>& blacklist) = 0;
   virtual void CheckAdminBlacklist() = 0;
 
   // Safe to call multiple times in a row.
   //
   // 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.
 class ExtensionService
     : public ExtensionServiceInterface,
       public ExternalExtensionProviderInterface::VisitorInterface,
       public NotificationObserver {
  public:
   // Information about a registered component extension.
   struct ComponentExtensionInfo {
@@ skipping 289 matching lines @@
   virtual void UpdateExtensionBlacklist(
       const std::vector<std::string>& blacklist) OVERRIDE;
 
   // Go through each extension and unload those that the network admin has
   // put on the blacklist (not to be confused with the Google managed blacklist
   // set of extensions.
   virtual void CheckAdminBlacklist() OVERRIDE;
 
   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_; }
 
   void set_show_extensions_prompts(bool enabled) {
     show_extensions_prompts_ = enabled;
   }
 
   bool show_extensions_prompts() {
     return show_extensions_prompts_;
@@ skipping 95 matching lines @@
   // |client| can be NULL for a silent install.
   scoped_refptr<CrxInstaller> MakeCrxInstaller(ExtensionInstallUI* client);
 
 #if defined(UNIT_TEST)
   void TrackTerminatedExtensionForTest(const Extension* extension) {
     TrackTerminatedExtension(extension);
   }
 #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 {
     // True if the background page is ready.
     bool background_page_ready;
 
     // True while the extension is being upgraded.
     bool being_upgraded;
 
     // Generic bag of runtime data that users can associate with extensions.
     PropertyBag property_bag;
 
     ExtensionRuntimeData();
     ~ExtensionRuntimeData();
   };
   typedef std::map<std::string, ExtensionRuntimeData> ExtensionRuntimeDataMap;
 
   struct NaClModuleInfo {
     NaClModuleInfo();
     ~NaClModuleInfo();
 
     GURL url;
     std::string mime_type;
   };
   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);
 
   // Look up an extension by ID, optionally including either or both of enabled
   // and disabled extensions.
   const Extension* GetExtensionByIdInternal(const std::string& id,
                                             bool include_enabled,
                                             bool include_disabled,
                                             bool include_terminated) const;
 
@@ skipping 161 matching lines @@
   // windows registry and external_extensions.json.
   ProviderCollection external_extension_providers_;
 
   // Set to true by OnExternalExtensionUpdateUrlFound() when an external
   // extension URL is found.  Used in CheckForExternalUpdates() to see
   // if an update check is needed to install pending extensions.
   bool external_extension_url_added_;
 
   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,
                            InstallAppsAndCheckStorageProtection);
   DISALLOW_COPY_AND_ASSIGN(ExtensionService);
 };
 
 #endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_SERVICE_H_