Convert app_launch_index and page_index from int to StringOrdinal.

chrome/browser/extensions/extension_prefs.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/extension_prefs.h"
 
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/extensions/extension_pref_store.h"
@@ skipping 75 matching lines @@
 
 // A preference set by the web store to indicate login information for
 // purchased apps.
 const char kWebStoreLogin[] = "extensions.webstore_login";
 
 // A preference set by the the NTP to persist the desired launch container type
 // used for apps.
 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";
 
 // A preference for storing extra data sent in update checks for an extension.
 const char kUpdateUrlData[] = "update_url_data";
 
 // Whether the browser action is visible in the toolbar.
 const char kBrowserActionVisible[] = "browser_action_visible";
 
@@ skipping 281 matching lines @@
   const DictionaryValue* ext = GetExtensionPref(extension_id);
   ListValue* out = NULL;
   if (!ext || !ext->GetList(pref_key, &out))
     return false;
   if (out_value)
     *out_value = out;
 
   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,
     URLPatternSet* result,
     int valid_schemes) {
   const ListValue* value = NULL;
   if (!ReadExtensionPrefList(extension_id, pref_key, &value))
     return false;
 
   result->ClearPatterns();
@@ skipping 340 matching lines @@
     if (ext->GetList(kPrefOldGrantedHosts, &hosts)) {
       UpdateExtensionPref(
           *ext_id, explicit_hosts, hosts->DeepCopy());
 
       // We can get rid of the old one by setting it to an empty list.
       UpdateExtensionPref(*ext_id, kPrefOldGrantedHosts, new ListValue());
     }
   }
 }
 
+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));
   return ReadExtensionPrefPermissionSet(extension_id, kPrefGrantedPermissions);
 }
 
 void ExtensionPrefs::AddGrantedPermissions(
     const std::string& extension_id,
     const ExtensionPermissionSet* permissions) {
   CHECK(Extension::IdIsValid(extension_id));
@@ skipping 179 matching lines @@
        iter != extension_ids.end(); ++iter) {
     toolbar_order->Append(new StringValue(*iter));
   }
   SavePrefs();
 }
 
 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);
   DictionaryValue* extension_dict = update.Get();
   const base::Time install_time = GetCurrentTime();
   extension_dict->Set(kPrefState, Value::CreateIntegerValue(initial_state));
   extension_dict->Set(kPrefLocation,
                       Value::CreateIntegerValue(extension->location()));
   extension_dict->Set(kPrefFromWebStore,
                       Value::CreateBooleanValue(from_webstore));
@@ skipping 11 matching lines @@
       extension->path());
   extension_dict->Set(kPrefPath, Value::CreateStringValue(path));
   // We store prefs about LOAD extensions, but don't cache their manifest
   // since it may change on disk.
   if (extension->location() != Extension::LOAD) {
     extension_dict->Set(kPrefManifest,
                         extension->manifest_value()->DeepCopy());
   }
 
   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);
   content_settings_store_->RegisterExtension(
       id, install_time, initial_state == Extension::ENABLED);
 }
 
 void ExtensionPrefs::OnExtensionUninstalled(const std::string& extension_id,
                                             const Extension::Location& location,
                                             bool external_uninstall) {
@@ skipping 312 matching lines @@
     return true;
   }
   return false;
 }
 
 void ExtensionPrefs::SetWebStoreLogin(const std::string& login) {
   prefs_->SetString(kWebStoreLogin, 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,
       Source<ExtensionPrefs>(this),
       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) {
   return ReadExtensionPrefBoolean(extension_id, kPrefUserDraggedApp);
 }
 
 void ExtensionPrefs::SetAppDraggedByUser(const std::string& extension_id) {
   UpdateExtensionPref(extension_id, kPrefUserDraggedApp,
                       Value::CreateBooleanValue(true));
 }
@@ skipping 127 matching lines @@
   // are pruned when persisting the preferences to disk).
   for (ExtensionIdSet::iterator ext_id = extension_ids.begin();
        ext_id != extension_ids.end(); ++ext_id) {
     ScopedExtensionPrefUpdate update(prefs_, *ext_id);
     // This creates an empty dictionary if none is stored.
     update.Get();
   }
 
   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
   // (ExtensionPrefStores) about the winning values.
   for (ExtensionIdSet::iterator ext_id = extension_ids.begin();
        ext_id != extension_ids.end(); ++ext_id) {
     extension_pref_value_map_->RegisterExtension(
         *ext_id,
         GetInstallTime(*ext_id),
         !IsExtensionDisabled(*ext_id));
@@ skipping 154 matching lines @@
   prefs->RegisterListPref(prefs::kExtensionInstallAllowList,
                           PrefService::UNSYNCABLE_PREF);
   prefs->RegisterListPref(prefs::kExtensionInstallDenyList,
                           PrefService::UNSYNCABLE_PREF);
   prefs->RegisterListPref(prefs::kExtensionInstallForceList,
                           PrefService::UNSYNCABLE_PREF);
   prefs->RegisterStringPref(kWebStoreLogin,
                             std::string() /* default_value */,
                             PrefService::UNSYNCABLE_PREF);
 }