Convert app_launch_index and page_index from int to StringOrdinal.

chrome/browser/extensions/extension_prefs.cc

Index: chrome/browser/extensions/extension_prefs.cc
diff --git a/chrome/browser/extensions/extension_prefs.cc b/chrome/browser/extensions/extension_prefs.cc
index 92412645bc76dc65f1c8531c4d385367bb72c0f3..759593aaeab6d347b2bd1aacad9c04438a00c615 100644
--- a/chrome/browser/extensions/extension_prefs.cc
+++ b/chrome/browser/extensions/extension_prefs.cc
@@ -110,10 +110,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 kPrefAppLaunchIndex[] = "app_launcher_index_string_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 kPrefPageIndex[] = "page_index_string_ordinal";
 
 // A preference specifying if the user dragged the app on the NTP.
 const char kPrefUserDraggedApp[] = "user_dragged_app_ntp";
@@ -239,6 +241,30 @@ std::string JoinPrefs(std::string parent, const char* child) {
   return parent + "." + child;
 }
 
+// A simple structure used when sorting extension based on the order they

[Finnur, 2011/11/17 14:59:25]

extension -> apps?

[csharp, 2011/11/17 19:51:58]

Done.

+// appear on the NTP. This is used by the code that migrates the data
+// from integers to StringOrdinals.
+struct ExtensionNTPOldPosition {
+  const std::string* extension_id;
+  const int page_index;
+  const int app_launch_index;
+
+  ExtensionNTPOldPosition(const std::string& ext_id, int page, int app_launch)
+      : extension_id(&ext_id),
+        page_index(page),
+        app_launch_index(app_launch) {}
+};
+
+struct SortByNTPOldPosition {
+  bool operator() (const ExtensionNTPOldPosition& lhs,
+                   const ExtensionNTPOldPosition& rhs) const {
+    if (lhs.page_index != rhs.page_index)
+      return lhs.page_index < rhs.page_index;
+
+    return lhs.app_launch_index < rhs.app_launch_index;
+  }
+};
+
 }  // namespace
 
 ExtensionPrefs::ExtensionPrefs(
@@ -415,6 +441,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,
@@ -858,6 +895,63 @@ void ExtensionPrefs::MigratePermissions(const ExtensionIdSet& extension_ids) {
   }
 }
 
+void ExtensionPrefs::MigrateAppIndex(const ExtensionIdSet& extension_ids) {
+  if (extension_ids.begin() == extension_ids.end()) {
+    return;
+  }

[Finnur, 2011/11/17 14:59:25]

No braces for single line if clauses.

[csharp, 2011/11/17 19:51:58]

Done.

+
+  // Create a sorted set of all the extensions that need either their page
+  // or launch index converted.
+  std::set<ExtensionNTPOldPosition, SortByNTPOldPosition> apps_to_convert;
+  for (ExtensionIdSet::const_iterator ext_id = extension_ids.begin();
+       ext_id != extension_ids.end(); ++ext_id) {
+    int old_page_index(0);
+    int old_app_launch_index(0);

[Finnur, 2011/11/17 14:59:25]

nit: Even though you can, we never initialize ints this way in the codebase...
I'd just stick to the general way.

[csharp, 2011/11/17 19:51:58]

Done.

+
+    bool has_old_values = ReadExtensionPrefInteger(*ext_id,
+                                                   kPrefPageIndexDeprecated,
+                                                   &old_page_index);
+    has_old_values |= ReadExtensionPrefInteger(*ext_id,
+                                               kPrefAppLaunchIndexDeprecated,
+                                               &old_app_launch_index);
+
+    if (has_old_values) {
+      // Its ok to use both values, even if they weren't read, because this
+      // only affects the sorting, which can handle apps with the default
+      // integer values.
+      apps_to_convert.insert(ExtensionNTPOldPosition(
+          *ext_id, old_page_index, old_app_launch_index));
+    }
+  }
+
+  if (apps_to_convert.empty())
+    return;
+
+  // Create the new values and remove the old preferences.
+  for (std::set<ExtensionNTPOldPosition, SortByNTPOldPosition>::iterator ext =
+           apps_to_convert.begin(); ext != apps_to_convert.end(); ++ext) {
+    int old_page_index;
+    if (ReadExtensionPrefInteger(*(ext->extension_id),
+                                 kPrefPageIndexDeprecated,
+                                 &old_page_index)) {
+      SetPageIndex(*(ext->extension_id),
+                   PageIndexAsStringOrdinal(old_page_index));
+      UpdateExtensionPref(*(ext->extension_id), kPrefPageIndexDeprecated, NULL);
+    }
+
+    int old_app_launch_index;
+    if (ReadExtensionPrefInteger(*(ext->extension_id),
+                                 kPrefAppLaunchIndexDeprecated,
+                                 &old_app_launch_index)) {

[Finnur, 2011/11/17 14:59:25]

old_page_index is read and used above.
old_app_launch_index, on the other hand, is read but not used. 

Will this not change the order of apps, potentially?

[csharp, 2011/11/17 19:51:58]

I don't think the order can change, because set that we are iterating though
here is sorted so that when we are placing an app, all the ones before it have
already been placed, so we know it will go into the next open position.

This could break if we have positions that have no app, but are suppose to show
up as empty space in the NTP, but as far as I'm aware we don't have that.

On 2011/11/17 14:59:25, Finnur wrote:

+      StringOrdinal page = GetPageIndex(*(ext->extension_id));
+      SetAppLaunchIndex(*(ext->extension_id),
+                        GetNextAppLaunchIndex(page));
+      UpdateExtensionPref(
+          *(ext->extension_id), kPrefAppLaunchIndexDeprecated, NULL);
+    }
+  }
+}
+
 ExtensionPermissionSet* ExtensionPrefs::GetGrantedPermissions(
     const std::string& extension_id) {
   CHECK(Extension::IdIsValid(extension_id));
@@ -1072,7 +1166,7 @@ void ExtensionPrefs::OnExtensionInstalled(
     const Extension* extension,
     Extension::State initial_state,
     bool from_webstore,
-    int page_index) {
+    StringOrdinal page_index) {
   const std::string& id = extension->id();
   CHECK(Extension::IdIsValid(id));
   ScopedExtensionPrefUpdate update(prefs_, id);
@@ -1104,12 +1198,15 @@ void ExtensionPrefs::OnExtensionInstalled(
   }
 
   if (extension->is_app()) {
-    if (page_index == -1)
+    if (!page_index.IsValid())
       page_index = GetNaturalAppPageIndex();
     extension_dict->Set(kPrefPageIndex,
-                        Value::CreateIntegerValue(page_index));
+                        Value::CreateStringValue(page_index.ToString()));
+    UpdatePageIndexMap(StringOrdinal(), page_index);
+
     extension_dict->Set(kPrefAppLaunchIndex,
-        Value::CreateIntegerValue(GetNextAppLaunchIndex(page_index)));
+                        Value::CreateStringValue(
+                            GetNextAppLaunchIndex(page_index).ToString()));
   }
   extension_pref_value_map_->RegisterExtension(
       id, install_time, initial_state == Extension::ENABLED);
@@ -1442,59 +1539,126 @@ 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;
+void ExtensionPrefs::GetAllAppsOnPageSorted(

[Finnur, 2011/11/17 14:59:25]

This doesn't really get all Apps on the page as much as all the app launch
indices (sorted)... No?

[csharp, 2011/11/17 19:51:58]

Correct, function name updated

On 2011/11/17 14:59:25, Finnur wrote:

+    const StringOrdinal& target_page_index,
+    std::set<StringOrdinal, StringOrdinal::Comparsion>* indexes_on_page) const {

[Finnur, 2011/11/17 14:59:25]

indexes -> indices?

[csharp, 2011/11/17 19:51:58]

Done.

+  const DictionaryValue* extensions = prefs_->GetDictionary(kExtensionsPref);
+  if (!extensions || !target_page_index.IsValid())
+    return;
+
+  for (DictionaryValue::key_iterator ext_it = extensions->begin_keys();
+       ext_it != extensions->end_keys(); ++ext_it) {
+    StringOrdinal page_index = GetPageIndex(*ext_it);
+    if (page_index.IsValid() && page_index.Equal(target_page_index)) {
+      StringOrdinal app_launch_index = GetAppLaunchIndex(*ext_it);
+      if (app_launch_index.IsValid())
+        indexes_on_page->insert(app_launch_index);
+    }
+  }
+}
+
+StringOrdinal ExtensionPrefs::GetFirstAppPage() const {
+  const DictionaryValue* extensions = prefs_->GetDictionary(kExtensionsPref);
+  if (!extensions)
+    return StringOrdinal();
+
+  if (page_index_map_.size() == 0)
+    return StringOrdinal::CreateValidOrdinal();
+
+  return page_index_map_.begin()->first;
+}
+
+StringOrdinal ExtensionPrefs::CreateFirstAppLaunchIndex(
+    const StringOrdinal& page_index) const {
+  std::set<StringOrdinal, StringOrdinal::Comparsion> apps_on_page;
+  GetAllAppsOnPageSorted(page_index, &apps_on_page);
 
-  return -1;
+  if (apps_on_page.empty()) {
+    return StringOrdinal::CreateValidOrdinal();
+  } else {
+    return apps_on_page.begin()->CreateBefore();
+  }

[Finnur, 2011/11/17 14:59:25]

nit: no braces around single-line clauses.

[csharp, 2011/11/17 19:51:58]

Done.

+}
+
+StringOrdinal ExtensionPrefs::GetAppLaunchIndex(
+    const std::string& extension_id) const {
+  std::string raw_value;
+  ReadExtensionPrefString(extension_id, kPrefAppLaunchIndex, &raw_value);
+  return StringOrdinal(raw_value);

[Finnur, 2011/11/17 14:59:25]

Is the idea here to return a blank ordinal if the pref read fails?

[csharp, 2011/11/17 19:51:58]

Yes, which is an invalid ordinal which does represent an unset pref. I've added
a comment to try and make this clearer.

On 2011/11/17 14:59:25, Finnur wrote:

 }
 
 void ExtensionPrefs::SetAppLaunchIndex(const std::string& extension_id,
-                                       int index) {
+                                       const StringOrdinal& index) {
   UpdateExtensionPref(extension_id, kPrefAppLaunchIndex,
-                      Value::CreateIntegerValue(index));
+                      Value::CreateStringValue(index.ToString()));
 }
 
-int ExtensionPrefs::GetNextAppLaunchIndex(int on_page) {
-  const DictionaryValue* extensions = prefs_->GetDictionary(kExtensionsPref);
-  if (!extensions)
-    return 0;
+StringOrdinal ExtensionPrefs::GetNextAppLaunchIndex(StringOrdinal on_page)
+    const {
+  std::set<StringOrdinal, StringOrdinal::Comparsion> apps_on_page;
+  GetAllAppsOnPageSorted(on_page, &apps_on_page);
 
-  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;
+  if (apps_on_page.empty()) {
+    return StringOrdinal::CreateValidOrdinal();
+  } else {
+    return apps_on_page.rbegin()->CreateAfter();
   }
-  return max_value + 1;
 }
 
-int ExtensionPrefs::GetNaturalAppPageIndex() {
+StringOrdinal ExtensionPrefs::GetNaturalAppPageIndex() const {
   const DictionaryValue* extensions = prefs_->GetDictionary(kExtensionsPref);
   if (!extensions)
-    return 0;
+    return StringOrdinal();
 
-  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;
+  if (page_index_map_.size() == 0) {
+    return StringOrdinal::CreateValidOrdinal();
   }
-  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;
+
+  std::map<StringOrdinal, int>::const_iterator it = page_index_map_.begin();
+  for (; it != page_index_map_.end(); ++it) {
+    if (it->second < kNaturalAppPageSize) {
+      return it->first;
+    }

[Finnur, 2011/11/17 14:59:25]

same here: no braces (also on line 1613, 1615).

[csharp, 2011/11/17 19:51:58]

Done.

   }
+
+  // Add a new page as all existing pages are full.
+  StringOrdinal last_element = page_index_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);
+    const std::vector<std::string>& extension_ids,
+    const std::string& moved_extension_id) {
+  std::vector<std::string>::const_iterator it =
+      find(extension_ids.begin(), extension_ids.end(), moved_extension_id);
+  size_t position = std::distance(extension_ids.begin(), it);
+
+  // We need at least 2 apps for the moved apps old value to be invalidated.
+  if (extension_ids.size() >= 2) {
+    if (position == 0U) {
+      // The app is moving to the start of the group, so we need to get the
+      // previous first StringOrdinal and create the new value before it.
+      ++it;
+      SetAppLaunchIndex(moved_extension_id,
+                        GetAppLaunchIndex(*it).CreateBefore());
+    } else if (position == extension_ids.size() - 1U) {
+      // The app is moving to the end of the group, so we need to get the
+      // previous last StringOrdinal and create the new value after it.
+      --it;
+      SetAppLaunchIndex(moved_extension_id,
+                        GetAppLaunchIndex(*it).CreateAfter());
+    } else {
+      // The app is going into a middle position, which means there are valid
+      // StringOrdinals before and after it. We get the new adjacent
+      // StringOrdinals and create our new value between them.
+      ++it;
+      StringOrdinal value_after = GetAppLaunchIndex(*it);
+      advance(it, -2);
+      StringOrdinal value_before = GetAppLaunchIndex(*it);
+      SetAppLaunchIndex(moved_extension_id,
+                        value_before.CreateBetween(value_after));
+    }
+  }
 
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_EXTENSION_LAUNCHER_REORDERED,
@@ -1502,21 +1666,84 @@ void ExtensionPrefs::SetAppLauncherOrder(
       content::NotificationService::NoDetails());
 }
 
-int ExtensionPrefs::GetPageIndex(const std::string& extension_id) {
-  int value = -1;
-  ReadExtensionPrefInteger(extension_id, kPrefPageIndex, &value);
-  return value;
+StringOrdinal ExtensionPrefs::GetPageIndex(const std::string& extension_id)
+    const {
+  std::string raw_string_data;
+  ReadExtensionPrefString(extension_id, kPrefPageIndex, &raw_string_data);
+  return StringOrdinal(raw_string_data);

[Finnur, 2011/11/17 14:59:25]

Same here: Is the idea here to return a blank ordinal if the pref read fails?

[csharp, 2011/11/17 19:51:58]

Yes, and comment added to clarify.

On 2011/11/17 14:59:25, Finnur wrote:

 }
 
-void ExtensionPrefs::SetPageIndex(const std::string& extension_id, int index) {
+void ExtensionPrefs::SetPageIndex(const std::string& extension_id,
+                                  const StringOrdinal& page_index) {
+  UpdatePageIndexMap(GetPageIndex(extension_id), page_index);
   UpdateExtensionPref(extension_id, kPrefPageIndex,
-                      Value::CreateIntegerValue(index));
+                      Value::CreateStringValue(page_index.ToString()));
 }
 
 void ExtensionPrefs::ClearPageIndex(const std::string& extension_id) {
+  UpdatePageIndexMap(GetPageIndex(extension_id), StringOrdinal());
   UpdateExtensionPref(extension_id, kPrefPageIndex, NULL);
 }
 
+void ExtensionPrefs::InitializePageIndexMap(
+    const ExtensionIdSet& extension_ids) {
+  for (ExtensionIdSet::const_iterator ext_it = extension_ids.begin();
+       ext_it != extension_ids.end(); ++ext_it) {
+    UpdatePageIndexMap(StringOrdinal(), GetPageIndex(*ext_it));

[Finnur, 2011/11/17 14:59:25]

Are there cases here where you can end up with {StringOrdinal(),
StringOrdinal()} ?

[csharp, 2011/11/17 19:51:58]

It is possible, but the map only updates for valid ordinals. If either input is
invalid it has no affect on the mapping (if the other value is valid it will
still affect the map by itself).

On 2011/11/17 14:59:25, Finnur wrote:

+  }
+}
+
+void ExtensionPrefs::UpdatePageIndexMap(const StringOrdinal& old_value,
+                                        const StringOrdinal& new_value) {
+  if (new_value.IsValid())
+    ++page_index_map_[new_value];
+
+  if (old_value.IsValid()) {
+    --page_index_map_[old_value];
+
+    if (page_index_map_[old_value] == 0)
+      page_index_map_.erase(old_value);
+  }
+}
+
+int ExtensionPrefs::PageIndexAsInteger(const StringOrdinal& page_index) const {
+  if (!page_index.IsValid())
+    return -1;
+
+  std::map<StringOrdinal, int>::const_iterator it =
+      page_index_map_.find(page_index);
+  if (it != page_index_map_.end()) {
+    return std::distance(page_index_map_.begin(), it);
+  } else {
+    return -1;
+  }
+}
+
+StringOrdinal ExtensionPrefs::PageIndexAsStringOrdinal(size_t page_index) {
+  const DictionaryValue* extensions = prefs_->GetDictionary(kExtensionsPref);
+  if (!extensions)
+    return StringOrdinal();
+
+  if (page_index_map_.size() == 0) {
+    return StringOrdinal::CreateValidOrdinal();
+  }

[Finnur, 2011/11/17 14:59:25]

nit: You don't need braces, your teeth look fine...

[csharp, 2011/11/17 19:51:58]

Done and :/

[Finnur, 2011/11/18 09:35:15]

Awww... isn't that smile lovely? :)

On 2011/11/17 19:51:58, csharp wrote:

+
+  if (page_index < page_index_map_.size()) {
+    std::map<StringOrdinal, int, StringOrdinal::Comparsion>::const_iterator it =
+        page_index_map_.begin();
+    advance(it, page_index);
+
+    return it->first;
+  } else {
+    // Create new pages until we reach the desired index.
+    while (page_index >= page_index_map_.size()) {
+      StringOrdinal new_page = page_index_map_.rbegin()->first.CreateAfter();
+      page_index_map_[new_page] = 0;
+    }
+    return page_index_map_.rbegin()->first;
+  }
+}
+
 bool ExtensionPrefs::WasAppDraggedByUser(const std::string& extension_id) {
   return ReadExtensionPrefBoolean(extension_id, kPrefUserDraggedApp);
 }
@@ -1661,7 +1888,9 @@ void ExtensionPrefs::InitPrefStore(bool extensions_disabled) {
   }
 
   FixMissingPrefs(extension_ids);
+  InitializePageIndexMap(extension_ids);
   MigratePermissions(extension_ids);
+  MigrateAppIndex(extension_ids);
 
   // Store extension controlled preference values in the
   // |extension_pref_value_map_|, which then informs the subscribers