base: Fix the TODO in ListValue::Remove().

Change the return type of Remove() to bool, and add a size_t output parameter.

BUG=None
TEST=None

R=evan@chromium.org

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

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

base/values.cc

@@ -818,21 +818,19 @@ bool ListValue::Remove(size_t index, Value** out_value) {
   return true;
 }
 
-int ListValue::Remove(const Value& value) {
+bool ListValue::Remove(const Value& value, size_t* index) {
   for (ValueVector::iterator i(list_.begin()); i != list_.end(); ++i) {
     if ((*i)->Equals(&value)) {
-      size_t index = i - list_.begin();
+      size_t previous_index = i - list_.begin();
       delete *i;
       list_.erase(i);
 
-      // TODO(anyone): Returning a signed int type here is just wrong.
-      // Change this interface to return a size_t.
-      DCHECK(index <= INT_MAX);
-      int return_index = static_cast<int>(index);
-      return return_index;
+      if (index)
+        *index = previous_index;
+      return true;
     }
   }
-  return -1;
+  return false;
 }
 
 void ListValue::Append(Value* in_value) {

base/values.h

@@ -402,8 +402,9 @@ class BASE_EXPORT ListValue : public Value {
   bool Remove(size_t index, Value** out_value);
 
   // Removes the first instance of |value| found in the list, if any, and
-  // deletes it.  Returns the index that it was located at (-1 for not present).
-  int Remove(const Value& value);
+  // deletes it. |index| is the location where |value| was found. Returns false
+  // if not found.
+  bool Remove(const Value& value, size_t* index);
 
   // Appends a Value to the end of the list.
   void Append(Value* in_value);

base/values_unittest.cc

@@ -236,7 +236,9 @@ TEST(ValuesTest, ListRemoval) {
     DeletionTestValue* value = new DeletionTestValue(&deletion_flag);
     list.Append(value);
     EXPECT_FALSE(deletion_flag);
-    EXPECT_EQ(0, list.Remove(*value));
+    size_t index = 0;
+    list.Remove(*value, &index);
+    EXPECT_EQ(0U, index);
     EXPECT_TRUE(deletion_flag);
     EXPECT_EQ(0U, list.GetSize());
   }

chrome/browser/chromeos/user_cros_settings_provider.cc

@@ -661,7 +661,7 @@ void UserCrosSettingsProvider::UnwhitelistUser(const std::string& email) {
   PrefService* prefs = g_browser_process->local_state();
   ListPrefUpdate cached_whitelist_update(prefs, kAccountsPrefUsers);
   StringValue email_value(email);
-  if (cached_whitelist_update->Remove(email_value) != -1)
+  if (cached_whitelist_update->Remove(email_value, NULL))
     prefs->ScheduleSavePersistentPrefs();
 }
 

chrome/browser/content_settings/content_settings_notification_provider.cc

@@ -281,7 +281,7 @@ void NotificationProvider::PersistPermissionChange(
     // Remove from one list and add to the other.
     if (is_allowed) {
       // Remove from the denied list.
-      if (denied_sites->Remove(*value) != -1)
+      if (denied_sites->Remove(*value, NULL))
         denied_changed = true;
 
       // Add to the allowed list.
@@ -289,7 +289,7 @@ void NotificationProvider::PersistPermissionChange(
         allowed_changed = true;
     } else {
       // Remove from the allowed list.
-      if (allowed_sites->Remove(*value) != -1)
+      if (allowed_sites->Remove(*value, NULL))
         allowed_changed = true;
 
       // Add to the denied list.
@@ -336,8 +336,8 @@ void NotificationProvider::ResetAllowedOrigin(const GURL& origin) {
     ListPrefUpdate update(prefs, prefs::kDesktopNotificationAllowedOrigins);
     ListValue* allowed_sites = update.Get();
     StringValue value(origin.spec());
-    int removed_index = allowed_sites->Remove(value);
-    DCHECK_NE(-1, removed_index) << origin << " was not allowed";
+    bool removed = allowed_sites->Remove(value, NULL);
+    DCHECK_NE(false, removed) << origin << " was not allowed";
   }
   prefs->ScheduleSavePersistentPrefs();
 }
@@ -353,8 +353,8 @@ void NotificationProvider::ResetBlockedOrigin(const GURL& origin) {
     ListPrefUpdate update(prefs, prefs::kDesktopNotificationDeniedOrigins);
     ListValue* denied_sites = update.Get();
     StringValue value(origin.spec());
-    int removed_index = denied_sites->Remove(value);
-    DCHECK_NE(-1, removed_index) << origin << " was not blocked";
+    bool removed = denied_sites->Remove(value, NULL);
+    DCHECK_NE(false, removed) << origin << " was not blocked";
   }
   prefs->ScheduleSavePersistentPrefs();
 }

chrome/browser/extensions/extension_web_ui.cc

@@ -332,9 +332,12 @@ void ExtensionWebUI::RegisterChromeURLOverrides(
 
 // static
 void ExtensionWebUI::UnregisterAndReplaceOverride(const std::string& page,
-    Profile* profile, ListValue* list, Value* override) {
-  int index = list->Remove(*override);
-  if (index == 0) {
+                                                  Profile* profile,
+                                                  ListValue* list,
+                                                  Value* override) {
+  size_t index = 0;
+  bool found = list->Remove(*override, &index);
+  if (found && index == 0) {
     // This is the active override, so we need to find all existing
     // tabs for this override and get them to reload the original URL.
     for (TabContentsIterator iterator; !iterator.done(); ++iterator) {

chrome/browser/extensions/external_policy_extension_loader_unittest.cc

@@ -85,7 +85,7 @@ class MockExternalPolicyExtensionProviderVisitor
 
     // Remove the extension from our list.
     StringValue ext_str(id + ";" + update_url.spec());
-    EXPECT_NE(-1, remaining_extensions->Remove(ext_str));
+    EXPECT_NE(false, remaining_extensions->Remove(ext_str, NULL));
   }
 
   virtual void OnExternalProviderReady() {

chrome/browser/translate/translate_prefs.cc

@@ -255,7 +255,7 @@ void TranslatePrefs::RemoveValueFromBlacklist(const char* pref_id,
       return;
     }
     StringValue string_value(value);
-    schedule_save = blacklist->Remove(string_value) != -1;
+    schedule_save = blacklist->Remove(string_value, NULL);
   }
   if (schedule_save)
     prefs_->ScheduleSavePersistentPrefs();