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 53bb982d1fe8d032ac5927aee306d379b69da930..9a9b55f7bcad01d7d133a2e468a6cb782a326adb 100644
--- a/chrome/browser/extensions/extension_prefs.cc
+++ b/chrome/browser/extensions/extension_prefs.cc
@@ -93,10 +93,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 kPrefAppLaunchIndexDepreciated[] = "app_launcher_index";

[Mihai Parparita -not on Chrome, 2011/10/07 23:15:12]

Typo (use "Deprecated" instead of "Depreciated"). Applies below too.

[csharp, 2011/11/11 18:13:32]

Done.

+const char kPrefAppLaunchIndex[] = "app_launcher_index_str";
 
 // A preference determining the page on which an app appears in the NTP.
-const char kPrefPageIndex[] = "page_index";
+const char kPrefPageIndexDepreciated[] = "page_index";
+const char kPrefPageIndex[] = "page_index_str";
 
 // A preference specifying if the user dragged the app on the NTP.
 const char kPrefUserDraggedApp[] = "user_dragged_app_ntp";
@@ -398,6 +400,16 @@ bool ExtensionPrefs::ReadExtensionPrefList(
   return true;
 }
 
+bool ExtensionPrefs::ReadExtensionPrefString(
+    const std::string& extension_id, const std::string& pref_key,
+    std::string* out_value) {
+  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,
@@ -758,6 +770,30 @@ void ExtensionPrefs::MigratePermissions(const ExtensionIdSet& extension_ids) {
   }
 }
 
+void ExtensionPrefs::MigrateAppIndex(const ExtensionIdSet& extension_ids) {
+  for (ExtensionIdSet::const_iterator ext_id = extension_ids.begin();
+       ext_id != extension_ids.end(); ++ext_id) {
+    std::string index;
+    if (ReadExtensionPrefString(*ext_id, kPrefPageIndex, &index)) {

[Mihai Parparita -not on Chrome, 2011/10/07 23:15:12]

I'm confused about this migration code, since this will only return true if you
already have a (new-style) string page index key, in which case you _don't_ want
to migrate, yet below you clobber it with the old value.

Also, on a successful migration you should remove the old pref keys.

[csharp, 2011/11/11 18:13:32]

Done.

+      int old_index;
+      if (ReadExtensionPrefInteger(*ext_id,
+                                   kPrefPageIndexDepreciated,
+                                   &old_index)) {
+        SetPageIndex(*ext_id, base::IntToString(old_index));
+      }
+    }
+
+    if (ReadExtensionPrefString(*ext_id, kPrefAppLaunchIndex, &index)) {
+      int old_index;
+      if (ReadExtensionPrefInteger(*ext_id,
+                                   kPrefPageIndexDepreciated,
+                                   &old_index)) {
+        SetAppLaunchIndex(*ext_id, base::IntToString(old_index));
+      }
+    }
+  }
+}
+
 ExtensionPermissionSet* ExtensionPrefs::GetGrantedPermissions(
     const std::string& extension_id) {
   CHECK(Extension::IdIsValid(extension_id));
@@ -957,7 +993,7 @@ void ExtensionPrefs::OnExtensionInstalled(
     const Extension* extension,
     Extension::State initial_state,
     bool from_webstore,
-    int page_index) {
+    std::string page_index) {
   const std::string& id = extension->id();
   CHECK(Extension::IdIsValid(id));
   ScopedExtensionPrefUpdate update(prefs_, id);
@@ -989,12 +1025,12 @@ void ExtensionPrefs::OnExtensionInstalled(
   }
 
   if (extension->is_app()) {
-    if (page_index == -1)
+    if (page_index == extension_misc::kUnsetIndex)
       page_index = GetNaturalAppPageIndex();
     extension_dict->Set(kPrefPageIndex,
-                        Value::CreateIntegerValue(page_index));
+                        Value::CreateStringValue(page_index));
     extension_dict->Set(kPrefAppLaunchIndex,
-        Value::CreateIntegerValue(GetNextAppLaunchIndex(page_index)));
+        Value::CreateStringValue(GetNextAppLaunchIndex(page_index)));
   }
   extension_pref_value_map_->RegisterExtension(
       id, install_time, initial_state == Extension::ENABLED);
@@ -1327,59 +1363,73 @@ void ExtensionPrefs::SetWebStoreLogin(const std::string& login) {
   SavePrefs();
 }
 
-int ExtensionPrefs::GetAppLaunchIndex(const std::string& extension_id) {
-  int value;
-  if (ReadExtensionPrefInteger(extension_id, kPrefAppLaunchIndex, &value))
+std::string ExtensionPrefs::GetAppLaunchIndex(const std::string& extension_id) {
+  std::string value;
+  if (ReadExtensionPrefString(extension_id, kPrefAppLaunchIndex, &value))
     return value;
 
-  return -1;
+  return extension_misc::kUnsetIndex;
 }
 
 void ExtensionPrefs::SetAppLaunchIndex(const std::string& extension_id,
-                                       int index) {
+                                       const std::string& index) {
   UpdateExtensionPref(extension_id, kPrefAppLaunchIndex,
-                      Value::CreateIntegerValue(index));
+                      Value::CreateStringValue(index));
 }
 
-int ExtensionPrefs::GetNextAppLaunchIndex(int on_page) {
+std::string ExtensionPrefs::GetNextAppLaunchIndex(std::string on_page) {
   const DictionaryValue* extensions = prefs_->GetDictionary(kExtensionsPref);
   if (!extensions)
-    return 0;
+    return "0";
 
-  int max_value = -1;
+  std::string max_value = extension_misc::kUnsetIndex;
   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)
+    std::string value = GetAppLaunchIndex(*extension_id);
+    std::string page = GetPageIndex(*extension_id);
+
+    int max_value_int, value_int;
+    base::StringToInt(max_value, &max_value_int);
+    base::StringToInt(value, &value_int);
+    if (page == on_page && max_value_int < value_int)
       max_value = value;
   }
-  return max_value + 1;
+
+  int new_value;
+  base::StringToInt(max_value, &new_value);

[akalin, 2011/10/07 17:31:16]

So all I'm seeing are conversions from int->string and string->int?

I thought I was clear earlier, but I guess not.

Basically, the point of using strings as indexes is to avoid the problem with
ints, which is basically that it's not true that there is an int between two
arbitrary ints (i.e., there's no dense ordering on the integers:
http://en.wikipedia.org/wiki/Dense_order ).

So an example of using strings would be to treat them as base-26 numbers in [0,
1), with a=0, b=1, ..., z=25.  Then a valid string index would be any string
that matches [a-z]*[b-z] (i.e., doesn't end in 'a').  So if you had 10 items,
they may have indices: b, d, f, h, j, n, q, s, u, w.  If you had to move the
item with index 'w' between 'b' and 'd', then you'd set its index to 'c'.  If
you then had to move the item with index 'u' between 'b' and 'c', then you'd set
its index to 'bm', since 'b' < 'bm' < 'c'.  If you then had to move the item
with index 's' before 'b', then you'd set its index to 'am', since 'am' < 'b'. 
And so on.

A problem with this scheme is that the string indices may grow to arbitrary
length.  But they do so slowly enough that it probably doesn't matter.

So you should either:

- make utility functions to manipulate and compare these string indices.
- make a class that's a thin wrapper around a string with methods to manipulate
and compare these string indices.

Then you can write code to migrate from ints to string indices.

+  ++new_value;
+
+  return base::IntToString(new_value);
 }
 
-int ExtensionPrefs::GetNaturalAppPageIndex() {
+std::string ExtensionPrefs::GetNaturalAppPageIndex() {
   const DictionaryValue* extensions = prefs_->GetDictionary(kExtensionsPref);
   if (!extensions)
-    return 0;
+    return "0";
 
-  std::map<int, int> page_counts;
+  std::map<std::string, 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;
+    std::string page_index = GetPageIndex(*extension_id);
+    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;
+
+  std::map<std::string, int>::const_iterator it = page_counts.begin();
+  for (int i = 0; it != page_counts.end(); ++it, ++i) {
+    if (it->second < kNaturalAppPageSize) {
+      return base::IntToString(i);
+    }
   }
+
+  // Add a new page
+  return base::IntToString(page_counts.size());
 }
 
 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);
+  for (size_t i = 0; i < extension_ids.size(); ++i) {
+    SetAppLaunchIndex(extension_ids.at(i), base::IntToString(i));
+  }
 
   NotificationService::current()->Notify(
       chrome::NOTIFICATION_EXTENSION_LAUNCHER_REORDERED,
@@ -1387,15 +1437,19 @@ void ExtensionPrefs::SetAppLauncherOrder(
       NotificationService::NoDetails());
 }
 
-int ExtensionPrefs::GetPageIndex(const std::string& extension_id) {
-  int value = -1;
-  ReadExtensionPrefInteger(extension_id, kPrefPageIndex, &value);
-  return value;
+std::string ExtensionPrefs::GetPageIndex(const std::string& extension_id) {
+  std::string value;
+  if (ReadExtensionPrefString(extension_id, kPrefPageIndex, &value)) {
+    return value;
+  }
+
+  return extension_misc::kUnsetIndex;
 }
 
-void ExtensionPrefs::SetPageIndex(const std::string& extension_id, int index) {
+void ExtensionPrefs::SetPageIndex(const std::string& extension_id,
+                                  const std::string& page_index) {
   UpdateExtensionPref(extension_id, kPrefPageIndex,
-                      Value::CreateIntegerValue(index));
+                      Value::CreateStringValue(page_index));
 }
 
 bool ExtensionPrefs::WasAppDraggedByUser(const std::string& extension_id) {
@@ -1543,6 +1597,7 @@ void ExtensionPrefs::InitPrefStore(bool extensions_disabled) {
 
   FixMissingPrefs(extension_ids);
   MigratePermissions(extension_ids);
+  MigrateAppIndex(extension_ids);
 
   // Store extension controlled preference values in the
   // |extension_pref_value_map_|, which then informs the subscribers