Remove Ordinals Setters and Getters from ExtensionService

Move the page and app launch setters and getters out of ExtensionService, moving their functionality into ExtensionSorting with the rest of the ordinal logic.


BUG=109769
TEST=ExtensionService unit tests pass and TwoClientAppsSync sync_integreation tests pass.


Review URL: http://codereview.chromium.org/9706017

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@127953 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 87 insertions, 94 deletions

diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index 5b1258b..6340724 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -445,6 +445,10 @@ ExtensionService::ExtensionService(Profile* profile,
   omnibox_icon_manager_.set_padding(gfx::Insets(0, kOmniboxIconPaddingLeft,
                                                 0, kOmniboxIconPaddingRight));
 
+  // Set this as the ExtensionService for extension sorting to ensure it
+  // cause syncs if required.
+  extension_prefs_->extension_sorting()->SetExtensionService(this);
+
   // How long is the path to the Extensions directory?
   UMA_HISTOGRAM_CUSTOM_COUNTS("Extensions.ExtensionRootPathLength",
                               install_directory_.value().length(), 0, 500, 100);
@@ -773,8 +777,9 @@ bool ExtensionService::UninstallExtension(
         IsIncognitoEnabled(extension_id),
         extension_prefs_->GetAppNotificationClientId(extension_id),
         extension_prefs_->IsAppNotificationDisabled(extension_id),
-        GetAppLaunchOrdinal(extension_id),
-        GetPageOrdinal(extension_id));
+        extension_prefs_->extension_sorting()->
+            GetAppLaunchOrdinal(extension_id),
+        extension_prefs_->extension_sorting()->GetPageOrdinal(extension_id));
     sync_change = extension_sync_data.GetSyncChange(SyncChange::ACTION_DELETE);
   }
 
@@ -1273,28 +1278,6 @@ bool ExtensionService::SyncBundle::HasPendingExtensionId(const std::string& id)
   return pending_sync_data.find(id) != pending_sync_data.end();
 }
 
-void ExtensionService::SyncExtensionChangeIfNeeded(const Extension& extension) {
-  SyncBundle* sync_bundle = GetSyncBundleForExtension(extension);
-  if (sync_bundle) {
-    ExtensionSyncData extension_sync_data(
-        extension,
-        IsExtensionEnabled(extension.id()),
-        IsIncognitoEnabled(extension.id()),
-        extension_prefs_->GetAppNotificationClientId(extension.id()),
-        extension_prefs_->IsAppNotificationDisabled(extension.id()),
-        GetAppLaunchOrdinal(extension.id()),
-        GetPageOrdinal(extension.id()));
-
-    SyncChangeList sync_change_list(1, 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))
@@ -1459,8 +1442,10 @@ void ExtensionService::GetSyncDataListHelper(
           IsIncognitoEnabled(extension.id()),
           extension_prefs_->GetAppNotificationClientId(extension.id()),
           extension_prefs_->IsAppNotificationDisabled(extension.id()),
-          GetAppLaunchOrdinal(extension.id()),
-          GetPageOrdinal(extension.id())));
+          extension_prefs_->extension_sorting()->
+              GetAppLaunchOrdinal(extension.id()),
+          extension_prefs_->extension_sorting()->
+              GetPageOrdinal(extension.id())));
     }
   }
 }
@@ -1522,8 +1507,12 @@ void ExtensionService::ProcessExtensionSyncData(
 
   if (extension_sync_data.app_launch_ordinal().IsValid() &&
       extension_sync_data.page_ordinal().IsValid()) {
-    SetAppLaunchOrdinal(id, extension_sync_data.app_launch_ordinal());
-    SetPageOrdinal(id, extension_sync_data.page_ordinal());
+    extension_prefs_->extension_sorting()->SetAppLaunchOrdinal(
+        id,
+        extension_sync_data.app_launch_ordinal());
+    extension_prefs_->extension_sorting()->SetPageOrdinal(
+        id,
+        extension_sync_data.page_ordinal());
   }
 
   if (extension_installed) {
@@ -1659,52 +1648,6 @@ bool ExtensionService::CanLoadInIncognito(const Extension* extension) const {
          IsIncognitoEnabled(extension->id());
 }
 
-StringOrdinal ExtensionService::GetAppLaunchOrdinal(
-    const std::string& extension_id) const {
-  return
-      extension_prefs_->extension_sorting()->GetAppLaunchOrdinal(extension_id);
-}
-
-void ExtensionService::SetAppLaunchOrdinal(
-    const std::string& extension_id,
-    const StringOrdinal& app_launch_ordinal) {
-  // Only apps should set this value, so we check that it is either an app or
-  // that it is not yet installed (so we can't be sure it is an app). It is
-  // possible to be setting this value through syncing before the app is
-  // installed.
-  const Extension* ext = GetExtensionById(extension_id, true);
-  DCHECK(!ext || ext->is_app());
-
-  extension_prefs_->extension_sorting()->SetAppLaunchOrdinal(
-      extension_id, app_launch_ordinal);
-
-  const Extension* extension = GetInstalledExtension(extension_id);
-  if (extension)
-    SyncExtensionChangeIfNeeded(*extension);
-}
-
-StringOrdinal ExtensionService::GetPageOrdinal(
-    const std::string& extension_id) const {
-  return extension_prefs_->extension_sorting()->GetPageOrdinal(extension_id);
-}
-
-void ExtensionService::SetPageOrdinal(const std::string& extension_id,
-                                      const StringOrdinal& page_ordinal) {
-  // Only apps should set this value, so we check that it is either an app or
-  // that it is not yet installed (so we can't be sure it is an app). It is
-  // possible to be setting this value through syncing before the app is
-  // installed.
-  const Extension* ext = GetExtensionById(extension_id, true);
-  DCHECK(!ext || ext->is_app());
-
-  extension_prefs_->extension_sorting()->SetPageOrdinal(
-      extension_id, page_ordinal);
-
-  const Extension* extension = GetInstalledExtension(extension_id);
-  if (extension)
-    SyncExtensionChangeIfNeeded(*extension);
-}
-
 void ExtensionService::OnExtensionMoved(
     const std::string& moved_extension_id,
     const std::string& predecessor_extension_id,
@@ -2010,6 +1953,29 @@ void ExtensionService::GarbageCollectExtensions() {
   }
 }
 
+void ExtensionService::SyncExtensionChangeIfNeeded(const Extension& extension) {
+  SyncBundle* sync_bundle = GetSyncBundleForExtension(extension);
+  if (sync_bundle) {
+    ExtensionSyncData extension_sync_data(
+        extension,
+        IsExtensionEnabled(extension.id()),
+        IsIncognitoEnabled(extension.id()),
+        extension_prefs_->GetAppNotificationClientId(extension.id()),
+        extension_prefs_->IsAppNotificationDisabled(extension.id()),
+        extension_prefs_->extension_sorting()->
+            GetAppLaunchOrdinal(extension.id()),
+        extension_prefs_->extension_sorting()->GetPageOrdinal(extension.id()));
+
+    SyncChangeList sync_change_list(1, 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());
+  }
+}
+
 void ExtensionService::OnLoadedInstalledExtensions() {
   if (updater_.get()) {
     updater_->Start();
diff --git a/chrome/browser/extensions/extension_service.h b/chrome/browser/extensions/extension_service.h
index 7f884ad..50e531d 100644
--- a/chrome/browser/extensions/extension_service.h
+++ b/chrome/browser/extensions/extension_service.h
@@ -124,6 +124,8 @@ class ExtensionServiceInterface : public SyncableService {
       const std::string& extension_id,
       extension_misc::UnloadedExtensionReason reason) = 0;
 
+  virtual void SyncExtensionChangeIfNeeded(const Extension& extension) = 0;
+
   virtual bool is_ready() = 0;
 };
 
@@ -226,17 +228,6 @@ class ExtensionService
   virtual void SetAppNotificationDisabled(const std::string& extension_id,
       bool value);
 
-  // Getters and setters for the position of Apps in the NTP. The setters
-  // will trigger a sync if needed.
-  // The getters return invalid StringOridinals for non-app extensions and
-  // setting ordinals for non-apps is an error.
-  StringOrdinal GetAppLaunchOrdinal(const std::string& extension_id) const;
-  void SetAppLaunchOrdinal(const std::string& extension_id,
-                           const StringOrdinal& app_launch_ordinal);
-  StringOrdinal GetPageOrdinal(const std::string& extension_id) const;
-  void SetPageOrdinal(const std::string& extension_id,
-                      const StringOrdinal& page_ordinal);
-
   // Updates the app launcher value for the moved extension so that it is now
   // located after the given predecessor and before the successor. This will
   // trigger a sync if needed. Empty strings are used to indicate no successor
@@ -374,6 +365,10 @@ class ExtensionService
   // Scan the extension directory and clean up the cruft.
   void GarbageCollectExtensions();
 
+  // Notifies Sync (if needed) of a newly-installed extension or a change to
+  // an existing extension.
+  virtual void SyncExtensionChangeIfNeeded(const Extension& extension) OVERRIDE;
+
   // The App that represents the web store.
   const Extension* GetWebStoreApp();
 
@@ -630,10 +625,6 @@ class ExtensionService
   };
   typedef std::list<NaClModuleInfo> NaClModuleInfoList;
 
-  // Notifies Sync (if needed) of a newly-installed extension or a change to
-  // an existing extension.
-  void SyncExtensionChangeIfNeeded(const Extension& extension);
-
   // Get the appropriate SyncBundle, given some representation of Sync data.
   SyncBundle* GetSyncBundleForExtension(const Extension& extension);
   SyncBundle* GetSyncBundleForExtensionSyncData(
diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc
index 65df9afe..20386f7 100644
--- a/chrome/browser/extensions/extension_service_unittest.cc
+++ b/chrome/browser/extensions/extension_service_unittest.cc
@@ -2008,7 +2008,6 @@ TEST_F(ExtensionServiceTest, EnsureCWSOrdinalsInitialized) {
                                     FilePath(FILE_PATH_LITERAL("web_store")));
   service_->Init();
 
-
   ExtensionSorting* sorting = service_->extension_prefs()->extension_sorting();
   EXPECT_TRUE(
       sorting->GetPageOrdinal(extension_misc::kWebStoreAppId).IsValid());
@@ -4119,7 +4118,8 @@ TEST_F(ExtensionServiceTest, GetSyncAppDataUserSettings) {
     EXPECT_TRUE(initial_ordinal.Equal(data.page_ordinal()));
   }
 
-  service_->SetAppLaunchOrdinal(app->id(), initial_ordinal.CreateAfter());
+  ExtensionSorting* sorting = service_->extension_prefs()->extension_sorting();
+  sorting->SetAppLaunchOrdinal(app->id(), initial_ordinal.CreateAfter());
   {
     SyncDataList list = service_->GetAllSyncData(syncable::APPS);
     ASSERT_EQ(list.size(), 1U);
@@ -4128,7 +4128,7 @@ TEST_F(ExtensionServiceTest, GetSyncAppDataUserSettings) {
     EXPECT_TRUE(initial_ordinal.Equal(data.page_ordinal()));
   }
 
-  service_->SetPageOrdinal(app->id(), initial_ordinal.CreateAfter());
+  sorting->SetPageOrdinal(app->id(), initial_ordinal.CreateAfter());
   {
     SyncDataList list = service_->GetAllSyncData(syncable::APPS);
     ASSERT_EQ(list.size(), 1U);
diff --git a/chrome/browser/extensions/extension_sorting.cc b/chrome/browser/extensions/extension_sorting.cc
index c306ca3..f9340dc 100644
--- a/chrome/browser/extensions/extension_sorting.cc
+++ b/chrome/browser/extensions/extension_sorting.cc
@@ -28,12 +28,18 @@ const char kPrefPageOrdinal[] = "page_ordinal";
 ExtensionSorting::ExtensionSorting(ExtensionScopedPrefs* extension_scoped_prefs,
                                    PrefService* pref_service)
     : extension_scoped_prefs_(extension_scoped_prefs),
-      pref_service_(pref_service) {
+      pref_service_(pref_service),
+      extension_service_(NULL) {
 }
 
 ExtensionSorting::~ExtensionSorting() {
 }
 
+void ExtensionSorting::SetExtensionService(
+    ExtensionServiceInterface* extension_service) {
+  extension_service_ = extension_service;
+}
+
 void ExtensionSorting::Initialize(
     const ExtensionPrefs::ExtensionIdSet& extension_ids) {
   InitializePageOrdinalMap(extension_ids);
@@ -277,6 +283,7 @@ void ExtensionSorting::SetAppLaunchOrdinal(
       extension_id,
       kPrefAppLaunchOrdinal,
       new_value);
+  SyncIfNeeded(extension_id);
 }
 
 StringOrdinal ExtensionSorting::CreateFirstAppLaunchOrdinal(
@@ -344,7 +351,7 @@ StringOrdinal ExtensionSorting::GetPageOrdinal(const std::string& extension_id)
 }
 
 void ExtensionSorting::SetPageOrdinal(const std::string& extension_id,
-                                    const StringOrdinal& new_page_ordinal) {
+                                      const StringOrdinal& new_page_ordinal) {
   // No work is required if the old and new values are the same.
   if (new_page_ordinal.EqualOrBothInvalid(GetPageOrdinal(extension_id)))
     return;
@@ -362,6 +369,7 @@ void ExtensionSorting::SetPageOrdinal(const std::string& extension_id,
       extension_id,
       kPrefPageOrdinal,
       new_value);
+  SyncIfNeeded(extension_id);
 }
 
 void ExtensionSorting::ClearOrdinals(const std::string& extension_id) {
@@ -490,3 +498,15 @@ void ExtensionSorting::RemoveOrdinalMapping(
     }
   }
 }
+
+void ExtensionSorting::SyncIfNeeded(const std::string& extension_id) {
+  if (extension_service_) {
+    const Extension* ext =
+        extension_service_->GetInstalledExtension(extension_id);
+
+    if (ext) {
+      CHECK(ext->is_app());
+      extension_service_->SyncExtensionChangeIfNeeded(*ext);
+    }
+  }
+}
diff --git a/chrome/browser/extensions/extension_sorting.h b/chrome/browser/extensions/extension_sorting.h
index 3542d51..b0bf38a 100644
--- a/chrome/browser/extensions/extension_sorting.h
+++ b/chrome/browser/extensions/extension_sorting.h
@@ -14,6 +14,7 @@
 #include "chrome/common/string_ordinal.h"
 
 class ExtensionScopedPrefs;
+class ExtensionServiceInterface;
 
 class ExtensionSorting {
  public:
@@ -21,6 +22,9 @@ class ExtensionSorting {
                             PrefService* pref_service);
   ~ExtensionSorting();
 
+  // Set up the ExtensionService to inform of changes that require syncing.
+  void SetExtensionService(ExtensionServiceInterface* extension_service);
+
   // Properly initialize ExtensionSorting internal values that require
   // |extension_ids|.
   void Initialize(const ExtensionPrefs::ExtensionIdSet& extension_ids);
@@ -133,8 +137,13 @@ class ExtensionSorting {
                             const StringOrdinal& page_ordinal,
                             const StringOrdinal& app_launch_ordinal);
 
+  // Syncs the extension if needed. It is an error to call this if the
+  // extension is not an application.
+  void SyncIfNeeded(const std::string& extension_id);
+
   ExtensionScopedPrefs* extension_scoped_prefs_;  // Weak, owns this instance.
   PrefService* pref_service_;  // Weak.
+  ExtensionServiceInterface* extension_service_;  // Weak.
 
   // A map of all the StringOrdinal page ordinals mapping to the collections of
   // app launch ordinals that exist on that page. This is used for mapping
diff --git a/chrome/browser/extensions/test_extension_service.cc b/chrome/browser/extensions/test_extension_service.cc
index bae3278..7ae9135 100644
--- a/chrome/browser/extensions/test_extension_service.cc
+++ b/chrome/browser/extensions/test_extension_service.cc
@@ -105,3 +105,8 @@ void TestExtensionService::UnloadExtension(
     extension_misc::UnloadedExtensionReason reason) {
   ADD_FAILURE();
 }
+
+void TestExtensionService::SyncExtensionChangeIfNeeded(
+    const Extension& extension) {
+  ADD_FAILURE();
+}
diff --git a/chrome/browser/extensions/test_extension_service.h b/chrome/browser/extensions/test_extension_service.h
index 0f6ffd0..21fb976 100644
--- a/chrome/browser/extensions/test_extension_service.h
+++ b/chrome/browser/extensions/test_extension_service.h
@@ -60,6 +60,8 @@ class TestExtensionService : public ExtensionServiceInterface {
   virtual void UnloadExtension(
       const std::string& extension_id,
       extension_misc::UnloadedExtensionReason reason) OVERRIDE;
+
+  virtual void SyncExtensionChangeIfNeeded(const Extension& extension) OVERRIDE;
 };
 
 #endif  // CHROME_BROWSER_EXTENSIONS_TEST_EXTENSION_SERVICE_H_