diff options
Diffstat (limited to 'chrome')
19 files changed, 762 insertions, 181 deletions
diff --git a/chrome/browser/extensions/app_process_apitest.cc b/chrome/browser/extensions/app_process_apitest.cc index 0ae9ead..8235feb 100644 --- a/chrome/browser/extensions/app_process_apitest.cc +++ b/chrome/browser/extensions/app_process_apitest.cc @@ -15,6 +15,7 @@ #include "chrome/common/chrome_switches.h" #include "chrome/common/extensions/extension.h" #include "chrome/common/extensions/extension_file_util.h" +#include "chrome/common/string_ordinal.h" #include "chrome/test/base/ui_test_utils.h" #include "content/browser/renderer_host/render_view_host.h" #include "content/browser/tab_contents/tab_contents.h" @@ -272,7 +273,8 @@ IN_PROC_BROWSER_TEST_F(AppApiTest, BookmarkAppGetsNormalProcess) { Extension::LOAD, Extension::FROM_BOOKMARK, &error)); - service->OnExtensionInstalled(extension, false, 0); + service->OnExtensionInstalled(extension, false, + StringOrdinal::CreateInitialOrdinal()); ASSERT_TRUE(extension.get()); ASSERT_TRUE(extension->from_bookmark()); diff --git a/chrome/browser/extensions/crx_installer.cc b/chrome/browser/extensions/crx_installer.cc index add5be9..9083b65 100644 --- a/chrome/browser/extensions/crx_installer.cc +++ b/chrome/browser/extensions/crx_installer.cc @@ -133,7 +133,6 @@ CrxInstaller::CrxInstaller(base::WeakPtr<ExtensionService> frontend_weak, extensions_enabled_(frontend_weak->extensions_enabled()), delete_source_(false), create_app_shortcut_(false), - page_index_(-1), frontend_weak_(frontend_weak), profile_(frontend_weak->profile()), client_(client), @@ -588,7 +587,7 @@ void CrxInstaller::ReportSuccessFromUIThread() { // Tell the frontend about the installation and hand off ownership of // extension_ to it. frontend_weak_->OnExtensionInstalled(extension_, is_gallery_install(), - page_index_); + page_ordinal_); NotifyCrxInstallComplete(extension_.get()); diff --git a/chrome/browser/extensions/crx_installer.h b/chrome/browser/extensions/crx_installer.h index cc937ac..192ef50 100644 --- a/chrome/browser/extensions/crx_installer.h +++ b/chrome/browser/extensions/crx_installer.h @@ -16,6 +16,7 @@ #include "chrome/browser/extensions/extension_install_ui.h" #include "chrome/browser/extensions/sandboxed_extension_unpacker.h" #include "chrome/common/extensions/extension.h" +#include "chrome/common/string_ordinal.h" #include "chrome/common/web_apps.h" class ExtensionService; @@ -177,8 +178,8 @@ class CrxInstaller install_cause_ = install_cause; } - void set_page_index(int page_index) { - page_index_ = page_index; + void set_page_ordinal(const StringOrdinal& page_ordinal) { + page_ordinal_ = page_ordinal; } Profile* profile() { return profile_; } @@ -271,8 +272,8 @@ class CrxInstaller // ExtensionService on success, or delete it on failure. scoped_refptr<const Extension> extension_; - // The index of the NTP apps page |extension_| will be shown on. - int page_index_; + // The ordinal of the NTP apps page |extension_| will be shown on. + StringOrdinal page_ordinal_; // A parsed copy of the unmodified original manifest, before any // transformations like localization have taken place. diff --git a/chrome/browser/extensions/extension_browsertest.cc b/chrome/browser/extensions/extension_browsertest.cc index 339a28c..6a0173f 100644 --- a/chrome/browser/extensions/extension_browsertest.cc +++ b/chrome/browser/extensions/extension_browsertest.cc @@ -92,7 +92,8 @@ const Extension* ExtensionBrowserTest::LoadExtensionWithOptions( // The call to OnExtensionInstalled ensures the other extension prefs // are set up with the defaults. service->extension_prefs()->OnExtensionInstalled( - extension, Extension::ENABLED, false, 0); + extension, Extension::ENABLED, false, + StringOrdinal::CreateInitialOrdinal()); // Toggling incognito or file access will reload the extension, so wait for // the reload and grab the new extension instance. The default state is diff --git a/chrome/browser/extensions/extension_prefs.cc b/chrome/browser/extensions/extension_prefs.cc index 0cd7c90..4357981 100644 --- a/chrome/browser/extensions/extension_prefs.cc +++ b/chrome/browser/extensions/extension_prefs.cc @@ -111,10 +111,12 @@ const char kWebStoreLogin[] = "extensions.webstore_login"; const char kPrefLaunchType[] = "launchType"; // A preference determining the order of which the apps appear on the NTP. -const char kPrefAppLaunchIndex[] = "app_launcher_index"; +const char kPrefAppLaunchIndexDeprecated[] = "app_launcher_index"; +const char kPrefAppLaunchOrdinal[] = "app_launcher_ordinal"; // A preference determining the page on which an app appears in the NTP. -const char kPrefPageIndex[] = "page_index"; +const char kPrefPageIndexDeprecated[] = "page_index"; +const char kPrefPageOrdinal[] = "page_ordinal"; // A preference specifying if the user dragged the app on the NTP. const char kPrefUserDraggedApp[] = "user_dragged_app_ntp"; @@ -417,6 +419,17 @@ bool ExtensionPrefs::ReadExtensionPrefList( return true; } +bool ExtensionPrefs::ReadExtensionPrefString( + const std::string& extension_id, const std::string& pref_key, + std::string* out_value) const { + const DictionaryValue* ext = GetExtensionPref(extension_id); + + if (!ext || !ext->GetString(pref_key, out_value)) + return false; + + return true; +} + bool ExtensionPrefs::ReadExtensionPrefURLPatternSet( const std::string& extension_id, const std::string& pref_key, @@ -864,6 +877,99 @@ void ExtensionPrefs::MigratePermissions(const ExtensionIdSet& extension_ids) { } } +void ExtensionPrefs::MigrateAppIndex(const ExtensionIdSet& extension_ids) { + if (extension_ids.empty()) + return; + + // Convert all the page index values to page ordinals. If there are any + // app launch values that need to be migrated, inserted them into a sorted + // set to be dealt with later. + typedef std::map<StringOrdinal, std::map<int, const std::string*>, + StringOrdinalLessThan> AppPositionToIdMapping; + AppPositionToIdMapping app_launches_to_convert; + for (ExtensionIdSet::const_iterator ext_id = extension_ids.begin(); + ext_id != extension_ids.end(); ++ext_id) { + int old_page_index = 0; + StringOrdinal page = GetPageOrdinal(*ext_id); + if (ReadExtensionPrefInteger(*ext_id, + kPrefPageIndexDeprecated, + &old_page_index)) { + // Some extensions have invalid page index, so we don't + // attempt to convert them. + if (old_page_index < 0) { + DLOG(WARNING) << "Extension " << *ext_id + << " has an invalid page index " << old_page_index + << ". Aborting attempt to convert its index."; + break; + } + + // Since we require all earlier StringOrdinals to already exist in order + // to properly convert from integers and we are iterating though them in + // no given order, we create earlier StringOrdinal values as required. + // This should be filled in by the time we are done with this loop. + if (page_ordinal_map_.empty()) + page_ordinal_map_[StringOrdinal::CreateInitialOrdinal()] = 0; + while (page_ordinal_map_.size() <= static_cast<size_t>(old_page_index)) { + StringOrdinal earlier_page = + page_ordinal_map_.rbegin()->first.CreateAfter(); + page_ordinal_map_[earlier_page] = 0; + } + + page = PageIntegerAsStringOrdinal(old_page_index); + SetPageOrdinal(*ext_id, page); + UpdateExtensionPref(*ext_id, kPrefPageIndexDeprecated, NULL); + } + + int old_app_launch_index = 0; + if (ReadExtensionPrefInteger(*ext_id, + kPrefAppLaunchIndexDeprecated, + &old_app_launch_index)) { + // We can't update the app launch index value yet, because we use + // GetNextAppLaunchOrdinal to get the new ordinal value and it requires + // all the ordinals with lower values to have already been migrated. + // A valid page ordinal is also required because otherwise there is + // no page to add the app to. + if (page.IsValid()) + app_launches_to_convert[page][old_app_launch_index] = &*ext_id; + + UpdateExtensionPref(*ext_id, kPrefAppLaunchIndexDeprecated, NULL); + } + } + + // Remove any empty pages that may have been added. This shouldn't occur, + // but double check here to prevent future problems with conversions between + // integers and StringOrdinals. + for (PageOrdinalMap::iterator it = page_ordinal_map_.begin(); + it != page_ordinal_map_.end();) { + if (it->second == 0) { + PageOrdinalMap::iterator prev_it = it; + ++it; + page_ordinal_map_.erase(prev_it); + } else { + ++it; + } + } + + if (app_launches_to_convert.empty()) + return; + + // Create the new app launch ordinals and remove the old preferences. Since + // the set is sorted, each time we migrate an apps index, we know that all of + // the remaining apps will appear further down the NTP than it or on a + // different page. + for (AppPositionToIdMapping::const_iterator page_it = + app_launches_to_convert.begin(); + page_it != app_launches_to_convert.end(); ++page_it) { + StringOrdinal page = page_it->first; + for (std::map<int, const std::string*>::const_iterator launch_it = + page_it->second.begin(); launch_it != page_it->second.end(); + ++launch_it) { + SetAppLaunchOrdinal(*(launch_it->second), + CreateNextAppLaunchOrdinal(page)); + } + } +} + ExtensionPermissionSet* ExtensionPrefs::GetGrantedPermissions( const std::string& extension_id) { CHECK(Extension::IdIsValid(extension_id)); @@ -1079,7 +1185,7 @@ void ExtensionPrefs::OnExtensionInstalled( const Extension* extension, Extension::State initial_state, bool from_webstore, - int page_index) { + const StringOrdinal& page_ordinal) { const std::string& id = extension->id(); CHECK(Extension::IdIsValid(id)); ScopedExtensionPrefUpdate update(prefs_, id); @@ -1111,13 +1217,12 @@ void ExtensionPrefs::OnExtensionInstalled( } if (extension->is_app()) { - if (page_index == -1) - page_index = GetNaturalAppPageIndex(); - extension_dict->Set(kPrefPageIndex, - Value::CreateIntegerValue(page_index)); - extension_dict->Set(kPrefAppLaunchIndex, - Value::CreateIntegerValue(GetNextAppLaunchIndex(page_index))); + StringOrdinal new_page_ordinal = + page_ordinal.IsValid() ? page_ordinal : GetNaturalAppPageOrdinal(); + SetPageOrdinal(id, new_page_ordinal); + SetAppLaunchOrdinal(id, CreateNextAppLaunchOrdinal(new_page_ordinal)); } + extension_pref_value_map_->RegisterExtension( id, install_time, initial_state == Extension::ENABLED); content_settings_store_->RegisterExtension( @@ -1142,6 +1247,37 @@ void ExtensionPrefs::OnExtensionUninstalled(const std::string& extension_id, } } +void ExtensionPrefs::OnExtensionMoved( + const std::string& moved_extension_id, + const std::string& predecessor_extension_id, + const std::string& successor_extension_id) { + // If there are no neighbors then no changes are required, so we can return. + if (predecessor_extension_id.empty() && successor_extension_id.empty()) + return; + + if (predecessor_extension_id.empty()) { + SetAppLaunchOrdinal( + moved_extension_id, + GetAppLaunchOrdinal(successor_extension_id).CreateBefore()); + } else if (successor_extension_id.empty()) { + SetAppLaunchOrdinal( + moved_extension_id, + GetAppLaunchOrdinal(predecessor_extension_id).CreateAfter()); + } else { + const StringOrdinal& predecessor_ordinal = + GetAppLaunchOrdinal(predecessor_extension_id); + const StringOrdinal& successor_ordinal = + GetAppLaunchOrdinal(successor_extension_id); + SetAppLaunchOrdinal(moved_extension_id, + predecessor_ordinal.CreateBetween(successor_ordinal)); + } + + content::NotificationService::current()->Notify( + chrome::NOTIFICATION_EXTENSION_LAUNCHER_REORDERED, + content::Source<ExtensionPrefs>(this), + content::NotificationService::NoDetails()); +} + void ExtensionPrefs::SetExtensionState(const std::string& extension_id, Extension::State state) { UpdateExtensionPref(extension_id, kPrefState, @@ -1449,79 +1585,180 @@ void ExtensionPrefs::SetWebStoreLogin(const std::string& login) { SavePrefs(); } -int ExtensionPrefs::GetAppLaunchIndex(const std::string& extension_id) { - int value; - if (ReadExtensionPrefInteger(extension_id, kPrefAppLaunchIndex, &value)) - return value; +StringOrdinal ExtensionPrefs::GetMinOrMaxAppLaunchOrdinalsOnPage( + const StringOrdinal& target_page_ordinal, + AppLaunchOrdinalReturn return_type) const { + const DictionaryValue* extensions = prefs_->GetDictionary(kExtensionsPref); + CHECK(extensions); + CHECK(target_page_ordinal.IsValid()); + + StringOrdinal return_value; + for (DictionaryValue::key_iterator ext_it = extensions->begin_keys(); + ext_it != extensions->end_keys(); ++ext_it) { + const StringOrdinal& page_ordinal = GetPageOrdinal(*ext_it); + if (page_ordinal.IsValid() && page_ordinal.Equal(target_page_ordinal)) { + const StringOrdinal& app_launch_ordinal = GetAppLaunchOrdinal(*ext_it); + if (app_launch_ordinal.IsValid()) { + if (!return_value.IsValid()) + return_value = app_launch_ordinal; + else if (return_type == ExtensionPrefs::MIN_ORDINAL && + app_launch_ordinal.LessThan(return_value)) + return_value = app_launch_ordinal; + else if (return_type == ExtensionPrefs::MAX_ORDINAL && + app_launch_ordinal.GreaterThan(return_value)) + return_value = app_launch_ordinal; + } + } + } + + return return_value; +} + +StringOrdinal ExtensionPrefs::GetAppLaunchOrdinal( + const std::string& extension_id) const { + std::string raw_value; + // If the preference read fails then raw_value will still be unset and we + // will return an invalid StringOrdinal to signal that no app launch ordinal + // was found. + ReadExtensionPrefString(extension_id, kPrefAppLaunchOrdinal, &raw_value); + return StringOrdinal(raw_value); +} - return -1; +void ExtensionPrefs::SetAppLaunchOrdinal(const std::string& extension_id, + const StringOrdinal& ordinal) { + UpdateExtensionPref(extension_id, kPrefAppLaunchOrdinal, + Value::CreateStringValue(ordinal.ToString())); } -void ExtensionPrefs::SetAppLaunchIndex(const std::string& extension_id, - int index) { - UpdateExtensionPref(extension_id, kPrefAppLaunchIndex, - Value::CreateIntegerValue(index)); +StringOrdinal ExtensionPrefs::CreateFirstAppLaunchOrdinal( + const StringOrdinal& page_ordinal) const { + const StringOrdinal& min_ordinal = + GetMinOrMaxAppLaunchOrdinalsOnPage(page_ordinal, + ExtensionPrefs::MIN_ORDINAL); + + if (min_ordinal.IsValid()) + return min_ordinal.CreateBefore(); + else + return StringOrdinal::CreateInitialOrdinal(); +} + +StringOrdinal ExtensionPrefs::CreateNextAppLaunchOrdinal( + const StringOrdinal& page_ordinal) const { + const StringOrdinal& max_ordinal = + GetMinOrMaxAppLaunchOrdinalsOnPage(page_ordinal, + ExtensionPrefs::MAX_ORDINAL); + + if (max_ordinal.IsValid()) + return max_ordinal.CreateAfter(); + else + return StringOrdinal::CreateInitialOrdinal(); } -int ExtensionPrefs::GetNextAppLaunchIndex(int on_page) { +StringOrdinal ExtensionPrefs::CreateFirstAppPageOrdinal() const { const DictionaryValue* extensions = prefs_->GetDictionary(kExtensionsPref); - if (!extensions) - return 0; + CHECK(extensions); - int max_value = -1; - for (DictionaryValue::key_iterator extension_id = extensions->begin_keys(); - extension_id != extensions->end_keys(); ++extension_id) { - int value = GetAppLaunchIndex(*extension_id); - int page = GetPageIndex(*extension_id); - if (page == on_page && value > max_value) - max_value = value; - } - return max_value + 1; + if (page_ordinal_map_.empty()) + return StringOrdinal::CreateInitialOrdinal(); + + return page_ordinal_map_.begin()->first; } -int ExtensionPrefs::GetNaturalAppPageIndex() { +StringOrdinal ExtensionPrefs::GetNaturalAppPageOrdinal() const { const DictionaryValue* extensions = prefs_->GetDictionary(kExtensionsPref); - if (!extensions) - return 0; + CHECK(extensions); - std::map<int, int> page_counts; - for (DictionaryValue::key_iterator extension_id = extensions->begin_keys(); - extension_id != extensions->end_keys(); ++extension_id) { - int page_index = GetPageIndex(*extension_id); - if (page_index >= 0) - page_counts[page_index] = page_counts[page_index] + 1; - } - for (int i = 0; ; i++) { - std::map<int, int>::const_iterator it = page_counts.find(i); - if (it == page_counts.end() || it->second < kNaturalAppPageSize) - return i; + if (page_ordinal_map_.empty()) + return StringOrdinal::CreateInitialOrdinal(); + + for (PageOrdinalMap::const_iterator it = page_ordinal_map_.begin(); + it != page_ordinal_map_.end(); ++it) { + if (it->second < kNaturalAppPageSize) + return it->first; } + + // Add a new page as all existing pages are full. + StringOrdinal last_element = page_ordinal_map_.rbegin()->first; + return last_element.CreateAfter(); } -void ExtensionPrefs::SetAppLauncherOrder( - const std::vector<std::string>& extension_ids) { - for (size_t i = 0; i < extension_ids.size(); ++i) - SetAppLaunchIndex(extension_ids.at(i), i); +StringOrdinal ExtensionPrefs::GetPageOrdinal(const std::string& extension_id) + const { + std::string raw_data; + // If the preference read fails then raw_data will still be unset and we will + // return an invalid StringOrdinal to signal that no page ordinal was found. + ReadExtensionPrefString(extension_id, kPrefPageOrdinal, &raw_data); + return StringOrdinal(raw_data); +} - content::NotificationService::current()->Notify( - chrome::NOTIFICATION_EXTENSION_LAUNCHER_REORDERED, - content::Source<ExtensionPrefs>(this), - content::NotificationService::NoDetails()); +void ExtensionPrefs::SetPageOrdinal(const std::string& extension_id, + const StringOrdinal& page_ordinal) { + UpdatePageOrdinalMap(GetPageOrdinal(extension_id), page_ordinal); + UpdateExtensionPref(extension_id, kPrefPageOrdinal, + Value::CreateStringValue(page_ordinal.ToString())); } -int ExtensionPrefs::GetPageIndex(const std::string& extension_id) { - int value = -1; - ReadExtensionPrefInteger(extension_id, kPrefPageIndex, &value); - return value; +void ExtensionPrefs::ClearPageOrdinal(const std::string& extension_id) { + UpdatePageOrdinalMap(GetPageOrdinal(extension_id), StringOrdinal()); + UpdateExtensionPref(extension_id, kPrefPageOrdinal, NULL); +} + +void ExtensionPrefs::InitializePageOrdinalMap( + const ExtensionIdSet& extension_ids) { + for (ExtensionIdSet::const_iterator ext_it = extension_ids.begin(); + ext_it != extension_ids.end(); ++ext_it) { + UpdatePageOrdinalMap(StringOrdinal(), GetPageOrdinal(*ext_it)); + } +} + +void ExtensionPrefs::UpdatePageOrdinalMap(const StringOrdinal& old_value, + const StringOrdinal& new_value) { + if (new_value.IsValid()) + ++page_ordinal_map_[new_value]; + + if (old_value.IsValid()) { + --page_ordinal_map_[old_value]; + + if (page_ordinal_map_[old_value] == 0) + page_ordinal_map_.erase(old_value); + } } -void ExtensionPrefs::SetPageIndex(const std::string& extension_id, int index) { - UpdateExtensionPref(extension_id, kPrefPageIndex, - Value::CreateIntegerValue(index)); +int ExtensionPrefs::PageStringOrdinalAsInteger( + const StringOrdinal& page_ordinal) const { + if (!page_ordinal.IsValid()) + return -1; + + PageOrdinalMap::const_iterator it = page_ordinal_map_.find(page_ordinal); + if (it != page_ordinal_map_.end()) { + return std::distance(page_ordinal_map_.begin(), it); + } else { + return -1; + } } -void ExtensionPrefs::ClearPageIndex(const std::string& extension_id) { - UpdateExtensionPref(extension_id, kPrefPageIndex, NULL); +StringOrdinal ExtensionPrefs::PageIntegerAsStringOrdinal(size_t page_index) + const { + // We shouldn't have a page_index that is more than 1 position away from the + // current end as that would imply we have empty pages which is not allowed. + CHECK_LE(page_index, page_ordinal_map_.size()); + + const DictionaryValue* extensions = prefs_->GetDictionary(kExtensionsPref); + if (!extensions) + return StringOrdinal(); + + if (page_index < page_ordinal_map_.size()) { + PageOrdinalMap::const_iterator it = page_ordinal_map_.begin(); + std::advance(it, page_index); + + return it->first; + + } else { + if (page_ordinal_map_.empty()) + return StringOrdinal::CreateInitialOrdinal(); + else + return page_ordinal_map_.rbegin()->first.CreateAfter(); + } } bool ExtensionPrefs::WasAppDraggedByUser(const std::string& extension_id) { @@ -1668,7 +1905,9 @@ void ExtensionPrefs::InitPrefStore(bool extensions_disabled) { } FixMissingPrefs(extension_ids); + InitializePageOrdinalMap(extension_ids); MigratePermissions(extension_ids); + MigrateAppIndex(extension_ids); // Store extension controlled preference values in the // |extension_pref_value_map_|, which then informs the subscribers diff --git a/chrome/browser/extensions/extension_prefs.h b/chrome/browser/extensions/extension_prefs.h index 75d7068..2bc1282 100644 --- a/chrome/browser/extensions/extension_prefs.h +++ b/chrome/browser/extensions/extension_prefs.h @@ -16,6 +16,7 @@ #include "chrome/browser/extensions/extension_prefs_scope.h" #include "chrome/browser/prefs/pref_service.h" #include "chrome/common/extensions/extension.h" +#include "chrome/common/string_ordinal.h" #include "googleurl/src/gurl.h" class ExtensionPrefValueMap; @@ -99,17 +100,26 @@ class ExtensionPrefs : public ExtensionContentSettingsStore::Observer { void SetToolbarOrder(const std::vector<std::string>& extension_ids); // Called when an extension is installed, so that prefs get created. - // If |page_index| is -1, and the then a page will be found for the App. + // If |page_ordinal| is an invalid ordinal, then a page will be found + // for the App. void OnExtensionInstalled(const Extension* extension, Extension::State initial_state, bool from_webstore, - int page_index); + const StringOrdinal& page_ordinal); // Called when an extension is uninstalled, so that prefs get cleaned up. void OnExtensionUninstalled(const std::string& extension_id, const Extension::Location& location, bool external_uninstall); + // Updates the app launcher value for the moved extension so that it is now + // located after the given predecessor and before the successor. + // Empty strings are used to indicate no successor or predecessor. + void OnExtensionMoved( + const std::string& moved_extension_id, + const std::string& predecessor_extension_id, + const std::string& successor_extension_id); + // Called to change the extension's state when it is enabled/disabled. void SetExtensionState(const std::string& extension_id, Extension::State); @@ -304,36 +314,55 @@ class ExtensionPrefs : public ExtensionContentSettingsStore::Observer { bool GetWebStoreLogin(std::string* result); void SetWebStoreLogin(const std::string& login); - // Get the application launch index for an extension with |extension_id|. This - // determines the order of which the applications appear on the New Tab Page. - // A value of 0 generally indicates top left. If the extension has no launch - // index a -1 value is returned. - int GetAppLaunchIndex(const std::string& extension_id); + // Get the application launch ordinal for an app with |extension_id|. This + // determines the order in which the app appears on the page it's on in the + // New Tab Page (Note that you can compare app launch ordinals only if the + // apps are on the same page). A string value close to |a*| generally + // indicates top left. If the extension has no launch ordinal, an invalid + // StringOrdinal is returned. + StringOrdinal GetAppLaunchOrdinal(const std::string& extension_id) const; + + // Sets a specific launch ordinal for an app with |extension_id|. + void SetAppLaunchOrdinal(const std::string& extension_id, + const StringOrdinal& ordinal); + + // Returns a StringOrdinal that is lower than any app launch ordinal for the + // given page. + StringOrdinal CreateFirstAppLaunchOrdinal(const StringOrdinal& page_ordinal) + const; - // Sets a specific launch index for an extension with |extension_id|. - void SetAppLaunchIndex(const std::string& extension_id, int index); + // Returns a StringOrdinal that is higher than any app launch ordinal for the + // given page. + StringOrdinal CreateNextAppLaunchOrdinal(const StringOrdinal& page_ordinal) + const; - // Gets the next available application launch index. This is 1 higher than the - // highest current application launch index found for the page |on_page|. - int GetNextAppLaunchIndex(int on_page); + // Returns a StringOrdinal that is lower than any existing page ordinal. + StringOrdinal CreateFirstAppPageOrdinal() const; - // Gets the page a new app should install to. Starts on page 0, and if there - // are N or more apps on it, tries to install on the next page. - int GetNaturalAppPageIndex(); + // Gets the page a new app should install to, which is the earliest non-full + // page. The returned ordinal may correspond to a page that doesn't yet exist + // if all pages are full. + StringOrdinal GetNaturalAppPageOrdinal() const; - // Sets the order the apps should be displayed in the app launcher. - void SetAppLauncherOrder(const std::vector<std::string>& extension_ids); + // Get the page ordinal for an app with |extension_id|. This determines + // which page an app will appear on in page-based NTPs. If the app has no + // page specified, an invalid StringOrdinal is returned. + StringOrdinal GetPageOrdinal(const std::string& extension_id) const; - // Get the application page index for an extension with |extension_id|. This - // determines which page an app will appear on in page-based NTPs. If - // the app has no page specified, -1 is returned. - int GetPageIndex(const std::string& extension_id); + // Sets a specific page ordinal for an app with |extension_id|. + void SetPageOrdinal(const std::string& extension_id, + const StringOrdinal& ordinal); - // Sets a specific page index for an extension with |extension_id|. - void SetPageIndex(const std::string& extension_id, int index); + // Removes the page ordinal for an app. + void ClearPageOrdinal(const std::string& extension_id); - // Removes the page index for an extension. - void ClearPageIndex(const std::string& extension_id); + // Convert the page StringOrdinal value to its integer equivalent. This takes + // O(# of apps) worst-case. + int PageStringOrdinalAsInteger(const StringOrdinal& page_ordinal) const; + + // Converts the page index integer to its StringOrdinal equivalent. This takes + // O(# of apps) worst-case. + StringOrdinal PageIntegerAsStringOrdinal(size_t page_index) const; // Returns true if the user repositioned the app on the app launcher via drag // and drop. @@ -409,6 +438,9 @@ class ExtensionPrefs : public ExtensionContentSettingsStore::Observer { virtual base::Time GetCurrentTime() const; private: + friend class ExtensionPrefsGetMinOrMaxAppLaunchOrdinalsOnPage; // Unit test. + friend class ExtensionPrefsMigrateAppIndex; // Unit test. + friend class ExtensionPrefsMigrateAppIndexInvalid; // Unit test. friend class ExtensionPrefsUninstallExtension; // Unit test. // ExtensionContentSettingsStore::Observer methods: @@ -457,6 +489,11 @@ class ExtensionPrefs : public ExtensionContentSettingsStore::Observer { const std::string& pref_key, const base::ListValue** out_value); + // Reads a string pref |pref_key| from extension with id |extension_id|. + bool ReadExtensionPrefString(const std::string& extension_id, + const std::string& pref_key, + std::string* out_value) const; + // Interprets the list pref, |pref_key| in |extension_id|'s preferences, as a // URLPatternSet. The |valid_schemes| specify how to parse the URLPatterns. bool ReadExtensionPrefURLPatternSet(const std::string& extension_id, @@ -514,17 +551,52 @@ class ExtensionPrefs : public ExtensionContentSettingsStore::Observer { // Migrates the permissions data in the pref store. void MigratePermissions(const ExtensionIdSet& extension_ids); + // Migrates the app launcher and page index values. + void MigrateAppIndex(const ExtensionIdSet& extension_ids); + // Checks whether there is a state pref for the extension and if so, whether // it matches |check_state|. bool DoesExtensionHaveState(const std::string& id, Extension::State check_state) const; + // An enum used by GetMinOrMaxAppLaunchOrdinalsOnPage to specify which + // value should be returned. + enum AppLaunchOrdinalReturn {MIN_ORDINAL, MAX_ORDINAL}; + + // This function returns the lowest ordinal on |page_ordinal| if + // |return_value| == AppLaunchOrdinalReturn::MIN_ORDINAL, otherwise it returns + // the largest ordinal on |page_ordinal|. If there are no apps on the page + // then an invalid StringOrdinal is returned. It is an error to call this + // function with an invalid |page_ordinal|. + StringOrdinal GetMinOrMaxAppLaunchOrdinalsOnPage( + const StringOrdinal& page_ordinal, + AppLaunchOrdinalReturn return_type) const; + + // Initialize the |page_ordinal_map_| with the page ordinals used by the + // given extensions. + void InitializePageOrdinalMap(const ExtensionIdSet& extension_ids); + + // Called when an application changes the value of its page ordinal so that + // |page_ordinal_map_| is aware that |old_value| page ordinal has been + // replace by the |new_value| page ordinal and adjusts its mapping + // accordingly. This works with valid and invalid StringOrdinals. + void UpdatePageOrdinalMap(const StringOrdinal& old_value, + const StringOrdinal& new_value); + // The pref service specific to this set of extension prefs. Owned by profile. PrefService* prefs_; // Base extensions install directory. FilePath install_directory_; + // A map of all the StringOrdinal page indices mapping to how often they are + // used, this is used for mapping the StringOrdinals to their integer + // equivalent as well as quick lookup of the sorted StringOrdinals. + // TODO(csharp) Convert this to a two-layer map to allow page ordinal lookup + // by id and vice versa. + typedef std::map<StringOrdinal, int, StringOrdinalLessThan> PageOrdinalMap; + PageOrdinalMap page_ordinal_map_; + // Weak pointer, owned by Profile. ExtensionPrefValueMap* extension_pref_value_map_; diff --git a/chrome/browser/extensions/extension_prefs_unittest.cc b/chrome/browser/extensions/extension_prefs_unittest.cc index 3d937d8..66dfbfd 100644 --- a/chrome/browser/extensions/extension_prefs_unittest.cc +++ b/chrome/browser/extensions/extension_prefs_unittest.cc @@ -10,11 +10,13 @@ #include "base/stringprintf.h" #include "chrome/browser/extensions/extension_prefs.h" #include "chrome/browser/extensions/test_extension_prefs.h" +#include "chrome/browser/extensions/extension_pref_value_map.h" #include "chrome/browser/prefs/pref_change_registrar.h" #include "chrome/browser/prefs/scoped_user_pref_update.h" #include "chrome/common/chrome_paths.h" #include "chrome/common/extensions/extension_constants.h" #include "chrome/common/extensions/extension_permission_set.h" +#include "chrome/common/string_ordinal.h" #include "content/public/browser/notification_details.h" #include "content/public/browser/notification_source.h" #include "content/test/notification_observer_mock.h" @@ -654,7 +656,8 @@ class ExtensionPrefsOnExtensionInstalled : public ExtensionPrefsTest { extension_ = prefs_.AddExtension("on_extension_installed"); EXPECT_FALSE(prefs()->IsExtensionDisabled(extension_->id())); prefs()->OnExtensionInstalled( - extension_.get(), Extension::DISABLED, false, -1); + extension_.get(), Extension::DISABLED, false, + StringOrdinal()); } virtual void Verify() { @@ -667,85 +670,109 @@ class ExtensionPrefsOnExtensionInstalled : public ExtensionPrefsTest { TEST_F(ExtensionPrefsOnExtensionInstalled, ExtensionPrefsOnExtensionInstalled) {} -class ExtensionPrefsAppLaunchIndex : public ExtensionPrefsTest { +class ExtensionPrefsAppLaunchOrdinal : public ExtensionPrefsTest { public: virtual void Initialize() { // No extensions yet. - EXPECT_EQ(0, prefs()->GetNextAppLaunchIndex(0)); + StringOrdinal page = StringOrdinal::CreateInitialOrdinal(); + EXPECT_TRUE(StringOrdinal::CreateInitialOrdinal().Equal( + prefs()->CreateNextAppLaunchOrdinal(page))); extension_ = prefs_.AddApp("on_extension_installed"); EXPECT_FALSE(prefs()->IsExtensionDisabled(extension_->id())); prefs()->OnExtensionInstalled(extension_.get(), Extension::ENABLED, - false, -1); + false, StringOrdinal()); } virtual void Verify() { - int launch_index = prefs()->GetAppLaunchIndex(extension_->id()); - // Extension should have been assigned a launch index > 0. - EXPECT_GT(launch_index, 0); - EXPECT_EQ(launch_index + 1, prefs()->GetNextAppLaunchIndex(0)); - // Set a new launch index of one higher and verify. - prefs()->SetAppLaunchIndex(extension_->id(), - prefs()->GetNextAppLaunchIndex(0)); - int new_launch_index = prefs()->GetAppLaunchIndex(extension_->id()); - EXPECT_EQ(launch_index + 1, new_launch_index); - - // This extension doesn't exist, so it should return -1. - EXPECT_EQ(-1, prefs()->GetAppLaunchIndex("foo")); - - // The second page doesn't have any apps so its next launch index should - // still be 0. - EXPECT_EQ(prefs()->GetNextAppLaunchIndex(1), 0); + StringOrdinal launch_ordinal = + prefs()->GetAppLaunchOrdinal(extension_->id()); + StringOrdinal page_ordinal = StringOrdinal::CreateInitialOrdinal(); + + // Extension should have been assigned a valid StringOrdinal. + EXPECT_TRUE(launch_ordinal.IsValid()); + EXPECT_TRUE(launch_ordinal.LessThan( + prefs()->CreateNextAppLaunchOrdinal(page_ordinal))); + // Set a new launch ordinal of and verify it comes after. + prefs()->SetAppLaunchOrdinal( + extension_->id(), + prefs()->CreateNextAppLaunchOrdinal(page_ordinal)); + StringOrdinal new_launch_ordinal = + prefs()->GetAppLaunchOrdinal(extension_->id()); + EXPECT_TRUE(launch_ordinal.LessThan(new_launch_ordinal)); + + // This extension doesn't exist, so it should return an invalid + // StringOrdinal. + StringOrdinal invalid_app_launch_ordinal = + prefs()->GetAppLaunchOrdinal("foo"); + EXPECT_FALSE(invalid_app_launch_ordinal.IsValid()); + EXPECT_EQ(-1, prefs()->PageStringOrdinalAsInteger( + invalid_app_launch_ordinal)); + + // The second page doesn't have any apps so its next launch ordinal should + // be the first launch ordinal. + StringOrdinal next_page = page_ordinal.CreateAfter(); + StringOrdinal next_page_app_launch_ordinal = + prefs()->CreateNextAppLaunchOrdinal(next_page); + EXPECT_TRUE(next_page_app_launch_ordinal.Equal( + prefs()->CreateFirstAppLaunchOrdinal(next_page))); } private: scoped_refptr<Extension> extension_; }; -TEST_F(ExtensionPrefsAppLaunchIndex, ExtensionPrefsAppLaunchIndex) {} +TEST_F(ExtensionPrefsAppLaunchOrdinal, ExtensionPrefsAppLaunchOrdinal) {} -class ExtensionPrefsPageIndex : public ExtensionPrefsTest { +class ExtensionPrefsPageOrdinal : public ExtensionPrefsTest { public: virtual void Initialize() { - extension_ = prefs_.AddApp("page_index"); - // Install to page 3 (index 2). + extension_ = prefs_.AddApp("page_ordinal"); + // Install with a page preference. + StringOrdinal page = StringOrdinal::CreateInitialOrdinal(); prefs()->OnExtensionInstalled(extension_.get(), Extension::ENABLED, - false, 2); - EXPECT_EQ(2, prefs()->GetPageIndex(extension_->id())); + false, page); + EXPECT_TRUE(page.Equal(prefs()->GetPageOrdinal(extension_->id()))); + EXPECT_EQ(0, prefs()->PageStringOrdinalAsInteger(page)); - scoped_refptr<Extension> extension2 = prefs_.AddApp("page_index_2"); + scoped_refptr<Extension> extension2 = prefs_.AddApp("page_ordinal_2"); // Install without any page preference. prefs()->OnExtensionInstalled(extension_.get(), Extension::ENABLED, - false, -1); - EXPECT_EQ(0, prefs()->GetPageIndex(extension_->id())); + false, StringOrdinal()); + EXPECT_TRUE(prefs()->GetPageOrdinal(extension_->id()).IsValid()); } virtual void Verify() { - // Set the page index. - prefs()->SetPageIndex(extension_->id(), 1); - // Verify the page index. - EXPECT_EQ(1, prefs()->GetPageIndex(extension_->id())); + StringOrdinal old_page = prefs()->GetPageOrdinal(extension_->id()); + StringOrdinal new_page = old_page.CreateAfter(); + + // Set the page ordinal. + prefs()->SetPageOrdinal(extension_->id(), new_page); + // Verify the page ordinal. + EXPECT_TRUE(new_page.Equal(prefs()->GetPageOrdinal(extension_->id()))); + EXPECT_EQ(1, prefs()->PageStringOrdinalAsInteger(new_page)); - // This extension doesn't exist, so it should return -1. - EXPECT_EQ(-1, prefs()->GetPageIndex("foo")); + // This extension doesn't exist, so it should return an invalid + // StringOrdinal. + EXPECT_FALSE(prefs()->GetPageOrdinal("foo").IsValid()); } private: scoped_refptr<Extension> extension_; }; -TEST_F(ExtensionPrefsPageIndex, ExtensionPrefsPageIndex) {} +TEST_F(ExtensionPrefsPageOrdinal, ExtensionPrefsPageOrdinal) {} class ExtensionPrefsAppLocation : public ExtensionPrefsTest { public: virtual void Initialize() { extension_ = prefs_.AddExtension("not_an_app"); - // Non-apps should not have any app launch index or page index. + // Non-apps should not have any app launch ordinal or page ordinal. prefs()->OnExtensionInstalled(extension_.get(), Extension::ENABLED, - false, 0); + false, StringOrdinal()); } virtual void Verify() { - EXPECT_EQ(-1, prefs()->GetAppLaunchIndex(extension_->id())); - EXPECT_EQ(-1, prefs()->GetPageIndex(extension_->id())); + EXPECT_FALSE(prefs()->GetAppLaunchOrdinal(extension_->id()).IsValid()); + EXPECT_FALSE(prefs()->GetPageOrdinal(extension_->id()).IsValid()); } private: @@ -759,7 +786,7 @@ class ExtensionPrefsAppDraggedByUser : public ExtensionPrefsTest { extension_ = prefs_.AddExtension("on_extension_installed"); EXPECT_FALSE(prefs()->WasAppDraggedByUser(extension_->id())); prefs()->OnExtensionInstalled(extension_.get(), Extension::ENABLED, - false, -1); + false, StringOrdinal()); } virtual void Verify() { @@ -909,7 +936,8 @@ class ExtensionPrefsPreferencesBase : public ExtensionPrefsTest { Extension* extensions[] = {ext1_, ext2_, ext3_}; for (int i = 0; i < 3; ++i) { if (ext == extensions[i] && !installed[i]) { - prefs()->OnExtensionInstalled(ext, Extension::ENABLED, false, -1); + prefs()->OnExtensionInstalled(ext, Extension::ENABLED, + false, StringOrdinal()); installed[i] = true; break; } @@ -1216,3 +1244,162 @@ class ExtensionPrefsDisableExtensions int iteration_; }; TEST_F(ExtensionPrefsDisableExtensions, ExtensionPrefsDisableExtensions) {} + +// Tests the application index to ordinal migration code. This should be removed +// when the migrate code is taken out. +class ExtensionPrefsMigrateAppIndex : public ExtensionPrefsPreferencesBase { + public: + ExtensionPrefsMigrateAppIndex() {} + virtual ~ExtensionPrefsMigrateAppIndex() {} + virtual void Initialize() { + // A preference determining the order of which the apps appear on the NTP. + const char kPrefAppLaunchIndexDeprecated[] = "app_launcher_index"; + // A preference determining the page on which an app appears in the NTP. + const char kPrefPageIndexDeprecated[] = "page_index"; + + // Setup the deprecated preferences. + prefs()->UpdateExtensionPref(ext1_->id(), + kPrefAppLaunchIndexDeprecated, + Value::CreateIntegerValue(0)); + prefs()->UpdateExtensionPref(ext1_->id(), + kPrefPageIndexDeprecated, + Value::CreateIntegerValue(0)); + + prefs()->UpdateExtensionPref(ext2_->id(), + kPrefAppLaunchIndexDeprecated, + Value::CreateIntegerValue(1)); + prefs()->UpdateExtensionPref(ext2_->id(), + kPrefPageIndexDeprecated, + Value::CreateIntegerValue(0)); + + prefs()->UpdateExtensionPref(ext3_->id(), + kPrefAppLaunchIndexDeprecated, + Value::CreateIntegerValue(0)); + prefs()->UpdateExtensionPref(ext3_->id(), + kPrefPageIndexDeprecated, + Value::CreateIntegerValue(1)); + + // We insert the ids in reserve order so that we have to deal with the + // element on the 2nd page before the 1st page is seen. + ExtensionPrefs::ExtensionIdSet ids; + ids.push_back(ext3_->id()); + ids.push_back(ext2_->id()); + ids.push_back(ext1_->id()); + + prefs_.prefs()->MigrateAppIndex(ids); + } + virtual void Verify() { + StringOrdinal first_ordinal = StringOrdinal::CreateInitialOrdinal(); + + EXPECT_TRUE(first_ordinal.Equal(prefs()->GetAppLaunchOrdinal(ext1_->id()))); + EXPECT_TRUE(first_ordinal.LessThan( + prefs()->GetAppLaunchOrdinal(ext2_->id()))); + EXPECT_TRUE(first_ordinal.Equal(prefs()->GetAppLaunchOrdinal(ext3_->id()))); + + EXPECT_TRUE(first_ordinal.Equal(prefs()->GetPageOrdinal(ext1_->id()))); + EXPECT_TRUE(first_ordinal.Equal(prefs()->GetPageOrdinal(ext2_->id()))); + EXPECT_TRUE(first_ordinal.LessThan(prefs()->GetPageOrdinal(ext3_->id()))); + } +}; +TEST_F(ExtensionPrefsMigrateAppIndex, ExtensionPrefsMigrateAppIndex) {} + +// Tests the application index to ordinal migration code for values that +// shouldn't be converted. This should be removed when the migrate code +// is taken out. +// http://crbug.com/107376 +class ExtensionPrefsMigrateAppIndexInvalid + : public ExtensionPrefsPreferencesBase { + public: + ExtensionPrefsMigrateAppIndexInvalid() {} + virtual ~ExtensionPrefsMigrateAppIndexInvalid() {} + virtual void Initialize() { + // A preference determining the order of which the apps appear on the NTP. + const char kPrefAppLaunchIndexDeprecated[] = "app_launcher_index"; + // A preference determining the page on which an app appears in the NTP. + const char kPrefPageIndexDeprecated[] = "page_index"; + + // Setup the deprecated preference. + prefs()->UpdateExtensionPref(ext1_->id(), + kPrefAppLaunchIndexDeprecated, + Value::CreateIntegerValue(0)); + prefs()->UpdateExtensionPref(ext1_->id(), + kPrefPageIndexDeprecated, + Value::CreateIntegerValue(-1)); + + ExtensionPrefs::ExtensionIdSet ids; + ids.push_back(ext1_->id()); + + prefs_.prefs()->MigrateAppIndex(ids); + } + virtual void Verify() { + // Make sure that the invalid page_index wasn't converted over. + EXPECT_FALSE(prefs()->GetAppLaunchOrdinal(ext1_->id()).IsValid()); + } +}; +TEST_F(ExtensionPrefsMigrateAppIndexInvalid, + ExtensionPrefsMigrateAppIndexInvalid) {} + +class ExtensionPrefsGetMinOrMaxAppLaunchOrdinalsOnPage : + public ExtensionPrefsPreferencesBase { + public: + ExtensionPrefsGetMinOrMaxAppLaunchOrdinalsOnPage() {} + virtual ~ExtensionPrefsGetMinOrMaxAppLaunchOrdinalsOnPage() {} + virtual void Initialize() { + DictionaryValue simple_dict; + simple_dict.SetString(keys::kVersion, "1.0.0.0"); + simple_dict.SetString(keys::kName, "unused"); + simple_dict.SetString(keys::kApp, "true"); + simple_dict.SetString(keys::kLaunchLocalPath, "fake.html"); + + std::string error; + app1_scoped_ = Extension::Create( + prefs_.temp_dir().AppendASCII("app1_"), Extension::EXTERNAL_PREF, + simple_dict, Extension::STRICT_ERROR_CHECKS, &error); + prefs()->OnExtensionInstalled(app1_scoped_.get(), + Extension::ENABLED, + false, + StringOrdinal()); + + app2_scoped_ = Extension::Create( + prefs_.temp_dir().AppendASCII("app2_"), Extension::EXTERNAL_PREF, + simple_dict, Extension::STRICT_ERROR_CHECKS, &error); + prefs()->OnExtensionInstalled(app2_scoped_.get(), + Extension::ENABLED, + false, + StringOrdinal()); + } + virtual void Verify() { + StringOrdinal page = StringOrdinal::CreateInitialOrdinal(); + + StringOrdinal min = prefs()->GetMinOrMaxAppLaunchOrdinalsOnPage( + page, + ExtensionPrefs::MIN_ORDINAL); + StringOrdinal max = prefs()->GetMinOrMaxAppLaunchOrdinalsOnPage( + page, + ExtensionPrefs::MAX_ORDINAL); + EXPECT_TRUE(min.IsValid()); + EXPECT_TRUE(max.IsValid()); + EXPECT_TRUE(min.LessThan(max)); + + // Ensure that the min and max values aren't set for empty pages. + min = StringOrdinal(); + max = StringOrdinal(); + StringOrdinal empty_page = page.CreateAfter(); + EXPECT_FALSE(min.IsValid()); + EXPECT_FALSE(max.IsValid()); + min = prefs()->GetMinOrMaxAppLaunchOrdinalsOnPage( + empty_page, + ExtensionPrefs::MIN_ORDINAL); + max = prefs()->GetMinOrMaxAppLaunchOrdinalsOnPage( + empty_page, + ExtensionPrefs::MAX_ORDINAL); + EXPECT_FALSE(min.IsValid()); + EXPECT_FALSE(max.IsValid()); + } + + private: + scoped_refptr<Extension> app1_scoped_; + scoped_refptr<Extension> app2_scoped_; +}; +TEST_F(ExtensionPrefsGetMinOrMaxAppLaunchOrdinalsOnPage, + ExtensionPrefsGetMinOrMaxAppLaunchOrdinalsOnPage) {} diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc index 40fb7c7..c5fd28c 100644 --- a/chrome/browser/extensions/extension_service.cc +++ b/chrome/browser/extensions/extension_service.cc @@ -2043,7 +2043,9 @@ void ExtensionService::UpdateActiveExtensionsInCrashReporter() { } void ExtensionService::OnExtensionInstalled( - const Extension* extension, bool from_webstore, int page_index) { + const Extension* extension, + bool from_webstore, + const StringOrdinal& page_ordinal) { CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); // Ensure extension is deleted unless we transfer ownership. @@ -2100,7 +2102,7 @@ void ExtensionService::OnExtensionInstalled( extension, initial_enable ? Extension::ENABLED : Extension::DISABLED, from_webstore, - page_index); + page_ordinal); // Unpacked extensions default to allowing file access, but if that has been // overridden, don't reset the value. diff --git a/chrome/browser/extensions/extension_service.h b/chrome/browser/extensions/extension_service.h index 5ab869c..00fff2e 100644 --- a/chrome/browser/extensions/extension_service.h +++ b/chrome/browser/extensions/extension_service.h @@ -379,7 +379,9 @@ class ExtensionService // Called by the backend when an extension has been installed. void OnExtensionInstalled( - const Extension* extension, bool from_webstore, int page_index); + const Extension* extension, + bool from_webstore, + const StringOrdinal& page_ordinal); // Initializes the |extension|'s active permission set and disables the // extension if the privilege level has increased (e.g., due to an upgrade). diff --git a/chrome/browser/extensions/test_extension_prefs.cc b/chrome/browser/extensions/test_extension_prefs.cc index cccbdec..aa19c4e 100644 --- a/chrome/browser/extensions/test_extension_prefs.cc +++ b/chrome/browser/extensions/test_extension_prefs.cc @@ -143,7 +143,8 @@ scoped_refptr<Extension> TestExtensionPrefs::AddExtensionWithManifestAndFlags( EXPECT_TRUE(Extension::IdIsValid(extension->id())); prefs_->OnExtensionInstalled(extension, Extension::ENABLED, - extra_flags & Extension::FROM_WEBSTORE, 0); + extra_flags & Extension::FROM_WEBSTORE, + StringOrdinal::CreateInitialOrdinal()); return extension; } diff --git a/chrome/browser/extensions/unpacked_installer.cc b/chrome/browser/extensions/unpacked_installer.cc index 9bbea9a..eabbfe6 100644 --- a/chrome/browser/extensions/unpacked_installer.cc +++ b/chrome/browser/extensions/unpacked_installer.cc @@ -12,6 +12,7 @@ #include "chrome/browser/extensions/extension_service.h" #include "chrome/common/extensions/extension.h" #include "chrome/common/extensions/extension_file_util.h" +#include "chrome/common/string_ordinal.h" using content::BrowserThread; @@ -56,7 +57,7 @@ void SimpleExtensionLoadPrompt::ShowPrompt() { void SimpleExtensionLoadPrompt::InstallUIProceed() { if (service_weak_.get()) service_weak_->OnExtensionInstalled( - extension_, false, -1); // Not from web store. + extension_, false, StringOrdinal()); // Not from web store. delete this; } @@ -209,7 +210,7 @@ void UnpackedInstaller::OnLoaded( } service_weak_->OnExtensionInstalled(extension, false, // Not from web store. - -1); + StringOrdinal()); } } // namespace extensions diff --git a/chrome/browser/resources/ntp4/page_list_view.js b/chrome/browser/resources/ntp4/page_list_view.js index a5065b1..682e8fa 100644 --- a/chrome/browser/resources/ntp4/page_list_view.js +++ b/chrome/browser/resources/ntp4/page_list_view.js @@ -279,9 +279,9 @@ cr.define('ntp4', function() { return true; } - // Sort by launch index + // Sort by launch ordinal apps.sort(function(a, b) { - return a.app_launch_index - b.app_launch_index; + return a.app_launch_ordinal > b.app_launch_ordinal; }); // An app to animate (in case it was just installed). diff --git a/chrome/browser/sync/test/integration/sync_extension_helper.cc b/chrome/browser/sync/test/integration/sync_extension_helper.cc index 08d1e8b..f9c7875 100644 --- a/chrome/browser/sync/test/integration/sync_extension_helper.cc +++ b/chrome/browser/sync/test/integration/sync_extension_helper.cc @@ -13,6 +13,7 @@ #include "chrome/browser/extensions/pending_extension_manager.h" #include "chrome/browser/profiles/profile.h" #include "chrome/common/extensions/extension_constants.h" +#include "chrome/common/string_ordinal.h" #include "chrome/browser/sync/test/integration/sync_test.h" #include "chrome/browser/sync/test/integration/sync_datatype_helper.h" #include "testing/gtest/include/gtest/gtest.h" @@ -64,7 +65,7 @@ void SyncExtensionHelper::InstallExtension( ASSERT_TRUE(extension.get()) << "Could not get extension " << name << " (profile = " << profile << ")"; profile->GetExtensionService()->OnExtensionInstalled( - extension, extension->UpdatesFromGallery(), 0); + extension, extension->UpdatesFromGallery(), StringOrdinal()); } void SyncExtensionHelper::UninstallExtension( diff --git a/chrome/browser/ui/panels/base_panel_browser_test.cc b/chrome/browser/ui/panels/base_panel_browser_test.cc index 2afe673..0b66219 100644 --- a/chrome/browser/ui/panels/base_panel_browser_test.cc +++ b/chrome/browser/ui/panels/base_panel_browser_test.cc @@ -19,6 +19,7 @@ #include "chrome/browser/ui/tab_contents/tab_contents_wrapper.h" #include "chrome/common/chrome_paths.h" #include "chrome/common/chrome_switches.h" +#include "chrome/common/string_ordinal.h" #include "chrome/test/base/ui_test_utils.h" #include "content/browser/tab_contents/test_tab_contents.h" #include "content/public/browser/notification_service.h" @@ -379,6 +380,6 @@ scoped_refptr<Extension> BasePanelBrowserTest::CreateExtension( EXPECT_TRUE(extension.get()); EXPECT_STREQ("", error.c_str()); browser()->GetProfile()->GetExtensionService()-> - OnExtensionInstalled(extension.get(), false, -1); + OnExtensionInstalled(extension.get(), false, StringOrdinal()); return extension; } diff --git a/chrome/browser/ui/webui/ntp/app_launcher_handler.cc b/chrome/browser/ui/webui/ntp/app_launcher_handler.cc index f94da20..6d1b8e0 100644 --- a/chrome/browser/ui/webui/ntp/app_launcher_handler.cc +++ b/chrome/browser/ui/webui/ntp/app_launcher_handler.cc @@ -67,6 +67,10 @@ extension_misc::AppLaunchBucket ParseLaunchSource( } // namespace +AppLauncherHandler::AppInstallInfo::AppInstallInfo() {} + +AppLauncherHandler::AppInstallInfo::~AppInstallInfo() {} + AppLauncherHandler::AppLauncherHandler(ExtensionService* extension_service) : extension_service_(extension_service), ignore_changes_(false), @@ -158,27 +162,32 @@ void AppLauncherHandler::CreateAppInfo(const Extension* extension, if (notification) value->Set("notification", SerializeNotification(*notification)); - int app_launch_index = prefs->GetAppLaunchIndex(extension->id()); - if (app_launch_index == -1) { - // Make sure every app has a launch index (some predate the launch index). - // The webstore's app launch index is set to -2 to make sure it's first. - // The next time the user drags (any) app this will be set to something - // sane (i.e. >= 0). - app_launch_index = extension->id() == extension_misc::kWebStoreAppId ? - -2 : prefs->GetNextAppLaunchIndex(0); - prefs->SetAppLaunchIndex(extension->id(), app_launch_index); - } - value->SetInteger("app_launch_index", app_launch_index); - - int page_index = prefs->GetPageIndex(extension->id()); - if (page_index < 0) { - // Make sure every app has a page index (some predate the page index). + StringOrdinal page_ordinal = prefs->GetPageOrdinal(extension->id()); + if (!page_ordinal.IsValid()) { + // Make sure every app has a page ordinal (some predate the page ordinal). // The webstore app should be on the first page. - page_index = extension->id() == extension_misc::kWebStoreAppId ? - 0 : prefs->GetNaturalAppPageIndex(); - prefs->SetPageIndex(extension->id(), page_index); + page_ordinal = extension->id() == extension_misc::kWebStoreAppId ? + prefs->CreateFirstAppPageOrdinal() : prefs->GetNaturalAppPageOrdinal(); + prefs->SetPageOrdinal(extension->id(), page_ordinal); } - value->SetInteger("page_index", page_index); + // We convert the page_ordinal to an integer because the pages are referenced + // from within an array in the javascript code, which can't be easily + // changed to handle the StringOrdinal values, so we do the conversion here. + value->SetInteger("page_index", + prefs->PageStringOrdinalAsInteger(page_ordinal)); + + StringOrdinal app_launch_ordinal = + prefs->GetAppLaunchOrdinal(extension->id()); + if (!app_launch_ordinal.IsValid()) { + // Make sure every app has a launch ordinal (some predate the launch + // ordinal). The webstore's app launch ordinal is always set to the first + // position. + app_launch_ordinal = extension->id() == extension_misc::kWebStoreAppId ? + prefs->CreateFirstAppLaunchOrdinal(page_ordinal) : + prefs->CreateNextAppLaunchOrdinal(page_ordinal); + prefs->SetAppLaunchOrdinal(extension->id(), app_launch_ordinal); + } + value->SetString("app_launch_ordinal", app_launch_ordinal.ToString()); } WebUIMessageHandler* AppLauncherHandler::Attach(WebUI* web_ui) { @@ -327,7 +336,7 @@ void AppLauncherHandler::Observe(int type, } void AppLauncherHandler::FillAppDictionary(DictionaryValue* dictionary) { - // CreateAppInfo and ClearPageIndex can change the extension prefs. + // CreateAppInfo and ClearPageOrdinal can change the extension prefs. AutoReset<bool> auto_reset(&ignore_changes_, true); ListValue* list = new ListValue(); @@ -341,11 +350,11 @@ void AppLauncherHandler::FillAppDictionary(DictionaryValue* dictionary) { } else { // This is necessary because in some previous versions of chrome, we set a // page index for non-app extensions. Old profiles can persist this error, - // and this fixes it. If we don't fix it, GetNaturalAppPageIndex() doesn't - // work. See http://crbug.com/98325 + // and this fixes it. This caused GetNaturalAppPageIndex() to break + // (see http://crbug.com/98325) before it was an ordinal value. ExtensionPrefs* prefs = extension_service_->extension_prefs(); - if (prefs->GetPageIndex(extension->id()) != -1) - prefs->ClearPageIndex(extension->id()); + if (prefs->GetPageOrdinal(extension->id()).IsValid()) + prefs->ClearPageOrdinal(extension->id()); } } @@ -643,17 +652,26 @@ void AppLauncherHandler::HandleReorderApps(const ListValue* args) { CHECK(args->GetString(0, &dragged_app_id)); CHECK(args->GetList(1, &app_order)); - std::vector<std::string> extension_ids; + std::string predecessor_to_moved_ext; + std::string successor_to_moved_ext; for (size_t i = 0; i < app_order->GetSize(); ++i) { std::string value; - if (app_order->GetString(i, &value)) - extension_ids.push_back(value); + if (app_order->GetString(i, &value) && value == dragged_app_id) { + if (i > 0) + CHECK(app_order->GetString(i - 1, &predecessor_to_moved_ext)); + if (i + 1 < app_order->GetSize()) + CHECK(app_order->GetString(i + 1, &successor_to_moved_ext)); + break; + } } // Don't update the page; it already knows the apps have been reordered. AutoReset<bool> auto_reset(&ignore_changes_, true); extension_service_->extension_prefs()->SetAppDraggedByUser(dragged_app_id); - extension_service_->extension_prefs()->SetAppLauncherOrder(extension_ids); + extension_service_->extension_prefs()->OnExtensionMoved( + dragged_app_id, + predecessor_to_moved_ext, + successor_to_moved_ext); } void AppLauncherHandler::HandleSetPageIndex(const ListValue* args) { @@ -661,11 +679,14 @@ void AppLauncherHandler::HandleSetPageIndex(const ListValue* args) { double page_index; CHECK(args->GetString(0, &extension_id)); CHECK(args->GetDouble(1, &page_index)); + const StringOrdinal& page_ordinal = + extension_service_->extension_prefs()->PageIntegerAsStringOrdinal( + static_cast<size_t>(page_index)); // Don't update the page; it already knows the apps have been reordered. AutoReset<bool> auto_reset(&ignore_changes_, true); - extension_service_->extension_prefs()->SetPageIndex(extension_id, - static_cast<int>(page_index)); + extension_service_->extension_prefs()->SetPageOrdinal(extension_id, + page_ordinal); } void AppLauncherHandler::HandlePromoSeen(const ListValue* args) { @@ -698,6 +719,9 @@ void AppLauncherHandler::HandleGenerateAppForLink(const ListValue* args) { double page_index; CHECK(args->GetDouble(2, &page_index)); + const StringOrdinal& page_ordinal = + extension_service_->extension_prefs()->PageIntegerAsStringOrdinal( + static_cast<size_t>(page_index)); Profile* profile = Profile::FromWebUI(web_ui_); FaviconService* favicon_service = @@ -711,7 +735,7 @@ void AppLauncherHandler::HandleGenerateAppForLink(const ListValue* args) { install_info->is_bookmark_app = true; install_info->title = title; install_info->app_url = launch_url; - install_info->page_index = static_cast<int>(page_index); + install_info->page_ordinal = page_ordinal; FaviconService::Handle h = favicon_service->GetFaviconForURL( launch_url, history::FAVICON, &favicon_consumer_, @@ -787,7 +811,7 @@ void AppLauncherHandler::OnFaviconForApp(FaviconService::Handle handle, scoped_refptr<CrxInstaller> installer( CrxInstaller::Create(extension_service_, NULL)); - installer->set_page_index(install_info->page_index); + installer->set_page_ordinal(install_info->page_ordinal); installer->InstallWebApp(*web_app); attempted_bookmark_app_install_ = true; } @@ -803,12 +827,12 @@ void AppLauncherHandler::SetAppToBeHighlighted() { // static void AppLauncherHandler::RegisterUserPrefs(PrefService* pref_service) { - // TODO(csilv): We will want this to be a syncable preference instead. + // TODO(csharp): We will want this to be a syncable preference instead. pref_service->RegisterListPref(prefs::kNTPAppPageNames, PrefService::UNSYNCABLE_PREF); } -// statiic +// static void AppLauncherHandler::RecordWebStoreLaunch(bool promo_active) { UMA_HISTOGRAM_ENUMERATION(extension_misc::kAppLaunchHistogram, extension_misc::APP_LAUNCH_NTP_WEBSTORE, diff --git a/chrome/browser/ui/webui/ntp/app_launcher_handler.h b/chrome/browser/ui/webui/ntp/app_launcher_handler.h index 35079ce..4006d3b 100644 --- a/chrome/browser/ui/webui/ntp/app_launcher_handler.h +++ b/chrome/browser/ui/webui/ntp/app_launcher_handler.h @@ -15,6 +15,7 @@ #include "chrome/browser/prefs/pref_change_registrar.h" #include "chrome/common/extensions/extension.h" #include "chrome/common/extensions/extension_constants.h" +#include "chrome/common/string_ordinal.h" #include "content/browser/cancelable_request.h" #include "content/browser/webui/web_ui.h" #include "content/public/browser/notification_observer.h" @@ -114,10 +115,13 @@ class AppLauncherHandler : public WebUIMessageHandler, private: struct AppInstallInfo { + AppInstallInfo(); + ~AppInstallInfo(); + bool is_bookmark_app; string16 title; GURL app_url; - int page_index; + StringOrdinal page_ordinal; }; // Records a web store launch in the appropriate histograms. |promo_active| diff --git a/chrome/common/string_ordinal.cc b/chrome/common/string_ordinal.cc index f070026..069afd2 100644 --- a/chrome/common/string_ordinal.cc +++ b/chrome/common/string_ordinal.cc @@ -174,6 +174,10 @@ StringOrdinal::StringOrdinal() : string_ordinal_(""), is_valid_(false) { } +StringOrdinal StringOrdinal::CreateInitialOrdinal() { + return StringOrdinal(std::string(1, kMidDigit)); +} + bool StringOrdinal::IsValid() const { return is_valid_; } @@ -184,6 +188,12 @@ bool StringOrdinal::LessThan(const StringOrdinal& other) const { return string_ordinal_ < other.string_ordinal_; } +bool StringOrdinal::GreaterThan(const StringOrdinal& other) const { + CHECK(IsValid()); + CHECK(other.IsValid()); + return string_ordinal_ > other.string_ordinal_; +} + bool StringOrdinal::Equal(const StringOrdinal& other) const { CHECK(IsValid()); CHECK(other.IsValid()); @@ -238,3 +248,8 @@ std::string StringOrdinal::ToString() const { CHECK(IsValid()); return string_ordinal_; } + +bool StringOrdinalLessThan::operator() (const StringOrdinal& lhs, + const StringOrdinal& rhs) const { + return lhs.LessThan(rhs); +} diff --git a/chrome/common/string_ordinal.h b/chrome/common/string_ordinal.h index abe0fe4..f97bdf8 100644 --- a/chrome/common/string_ordinal.h +++ b/chrome/common/string_ordinal.h @@ -29,6 +29,11 @@ class StringOrdinal { // Creates an invalid StringOrdinal. StringOrdinal(); + // Creates a valid initial StringOrdinal, this is called to create the first + // element of StringOrdinal list (i.e. before we have any other values we can + // generate from). + static StringOrdinal CreateInitialOrdinal(); + bool IsValid() const; // All remaining functions can only be called if IsValid() holds. @@ -39,6 +44,9 @@ class StringOrdinal { // Returns true iff |*this| < |other|. bool LessThan(const StringOrdinal& other) const; + // Returns true iff |*this| > |other|. + bool GreaterThan(const StringOrdinal& other) const; + // Returns true iff |*this| == |other| (i.e. |*this| < |other| and // |other| < |*this| are both false). bool Equal(const StringOrdinal& other) const; @@ -69,4 +77,10 @@ class StringOrdinal { bool is_valid_; }; +// A helper class that can be used by STL containers that require sorting. +class StringOrdinalLessThan { + public: + bool operator() (const StringOrdinal& lhs, const StringOrdinal& rhs) const; +}; + #endif // CHROME_COMMON_STRING_ORDINAL_H_ diff --git a/chrome/common/string_ordinal_unittest.cc b/chrome/common/string_ordinal_unittest.cc index 8b95eab..f3bc922 100644 --- a/chrome/common/string_ordinal_unittest.cc +++ b/chrome/common/string_ordinal_unittest.cc @@ -29,6 +29,21 @@ TEST(StringOrdinalTest, LessThan) { EXPECT_FALSE(middle_value.LessThan(small_value)); } +TEST(StringOrdinalTest, GreaterThan) { + StringOrdinal small_value("b"); + StringOrdinal middle_value("n"); + StringOrdinal big_value("z"); + + EXPECT_TRUE(big_value.GreaterThan(small_value)); + EXPECT_TRUE(big_value.GreaterThan(middle_value)); + EXPECT_TRUE(middle_value.GreaterThan(small_value)); + + + EXPECT_FALSE(small_value.GreaterThan(middle_value)); + EXPECT_FALSE(small_value.GreaterThan(big_value)); + EXPECT_FALSE(middle_value.GreaterThan(big_value)); +} + // Tests the CreateBetween StringOrdinal function by calling // on the small_value with the large_value as the parameter and // vice-versa. |