Add base::Value::Remove{Key,Path}

This change introduces RemoveKey and RemovePath to base::Value. This
complements the existing Set and Find methods and deprecates the
currently existing base::DictionaryValue::Remove, RemovePath and
RemoveWithoutPathExpansion.

Bug: 646113
Change-Id: I964802e5ef1017b81b9accd4026b8d919a5b04b4
Reviewed-on: https://chromium-review.googlesource.com/637833
Commit-Queue: Jan Wilken Dörrie <[email protected]>
Reviewed-by: Brett Wilson <[email protected]>
Cr-Commit-Position: refs/heads/master@{#499511}

base/values.cc

@@ -289,6 +289,12 @@ const Value* Value::FindKeyOfType(StringPiece key, Type type) const {
   return result;
 }
 
+bool Value::RemoveKey(StringPiece key) {
+  CHECK(is_dict());
+  // NOTE: Can't directly return dict_->erase(key) due to MSVC warning C4800.
+  return dict_->erase(key) != 0;
+}
+
 Value* Value::SetKey(StringPiece key, Value value) {
   CHECK(is_dict());
   return ((*dict_)[key.as_string()] = std::make_unique<Value>(std::move(value)))
@@ -385,6 +391,28 @@ Value* Value::SetPath(span<const StringPiece> path, Value value) {
   return cur->SetKey(*cur_path, std::move(value));
 }
 
+bool Value::RemovePath(std::initializer_list<StringPiece> path) {
+  return RemovePath(make_span(path.begin(), path.size()));
+}
+
+bool Value::RemovePath(span<const StringPiece> path) {
+  if (!is_dict() || path.empty())
+    return false;
+
+  if (path.size() == 1)
+    return RemoveKey(path[0]);
+
+  auto found = dict_->find(path[0]);
+  if (found == dict_->end() || !found->second->is_dict())
+    return false;
+
+  bool removed = found->second->RemovePath(path.subspan(1));
+  if (removed && found->second->dict_->empty())
+    dict_->erase(found);
+
+  return removed;
+}
+
 Value::dict_iterator_proxy Value::DictItems() {
   CHECK(is_dict());
   return dict_iterator_proxy(&*dict_);

base/values.h

@@ -204,6 +204,16 @@ class BASE_EXPORT Value {
   // This overload is necessary to avoid ambiguity for const char* arguments.
   Value* SetKey(const char* key, Value value);
 
+  // This attemps to remove the value associated with |key|. In case of failure,
+  // e.g. the key does not exist, |false| is returned and the underlying
+  // dictionary is not changed. In case of success, |key| is deleted from the
+  // dictionary and the method returns |true|.
+  // Note: This fatally asserts if type() is not Type::DICTIONARY.
+  //
+  // Example:
+  //   bool success = RemoveKey("foo");
+  bool RemoveKey(StringPiece key);
+
   // Searches a hierarchy of dictionary values for a given value. If a path
   // of dictionaries exist, returns the item at that path. If any of the path
   // components do not exist or if any but the last path components are not
@@ -235,11 +245,12 @@ class BASE_EXPORT Value {
 
   // Sets the given path, expanding and creating dictionary keys as necessary.
   //
-  // The current value must be a dictionary. If path components do not exist,
-  // they will be created. If any but the last components matches a value that
-  // is not a dictionary, the function will fail (it will not overwrite the
-  // value) and return nullptr. The last path component will be unconditionally
-  // overwritten if it exists, and created if it doesn't.
+  // If the current value is not a dictionary, the function returns nullptr. If
+  // path components do not exist, they will be created. If any but the last
+  // components matches a value that is not a dictionary, the function will fail
+  // (it will not overwrite the value) and return nullptr. The last path
+  // component will be unconditionally overwritten if it exists, and created if
+  // it doesn't.
   //
   // Example:
   //   value.SetPath({"foo", "bar"}, std::move(myvalue));
@@ -249,6 +260,21 @@ class BASE_EXPORT Value {
   Value* SetPath(std::initializer_list<StringPiece> path, Value value);
   Value* SetPath(span<const StringPiece> path, Value value);
 
+  // Tries to remove a Value at the given path.
+  //
+  // If the current value is not a dictionary or any path components does not
+  // exist, this operation fails, leaves underlying Values untouched and returns
+  // |false|. In case intermediate dictionaries become empty as a result of this
+  // path removal, they will be removed as well.
+  //
+  // Example:
+  //   bool success = value.RemovePath({"foo", "bar"});
+  //
+  //   std::vector<StringPiece> components = ...
+  //   bool success = value.RemovePath(components);
+  bool RemovePath(std::initializer_list<StringPiece> path);
+  bool RemovePath(span<const StringPiece> path);
+
   using dict_iterator_proxy = detail::dict_iterator_proxy;
   using const_dict_iterator_proxy = detail::const_dict_iterator_proxy;
 
@@ -485,17 +511,22 @@ class BASE_EXPORT DictionaryValue : public Value {
   // |out_value|.  If |out_value| is NULL, the removed value will be deleted.
   // This method returns true if |path| is a valid path; otherwise it will
   // return false and the DictionaryValue object will be unchanged.
+  // DEPRECATED, use Value::RemovePath(path) instead.
   bool Remove(StringPiece path, std::unique_ptr<Value>* out_value);
 
   // Like Remove(), but without special treatment of '.'.  This allows e.g. URLs
   // to be used as paths.
+  // DEPRECATED, use Value::RemoveKey(key) instead.
   bool RemoveWithoutPathExpansion(StringPiece key,
                                   std::unique_ptr<Value>* out_value);
 
   // Removes a path, clearing out all dictionaries on |path| that remain empty
   // after removing the value at |path|.
+  // DEPRECATED, use Value::RemovePath(path) instead.
   bool RemovePath(StringPiece path, std::unique_ptr<Value>* out_value);
 
+  using Value::RemovePath;  // DictionaryValue::RemovePath shadows otherwise.
+
   // Makes a copy of |this| but doesn't include empty dictionaries and lists in
   // the copy.  This never returns NULL, even if |this| itself is empty.
   std::unique_ptr<DictionaryValue> DeepCopyWithoutEmptyChildren() const;

base/values_unittest.cc

@@ -661,6 +661,46 @@ TEST(ValuesTest, SetPath) {
   EXPECT_FALSE(found);
 }
 
+TEST(ValuesTest, RemoveKey) {
+  Value root(Value::Type::DICTIONARY);
+  root.SetKey("one", Value(123));
+
+  // Removal of missing key should fail.
+  EXPECT_FALSE(root.RemoveKey("two"));
+
+  // Removal of existing key should succeed.
+  EXPECT_TRUE(root.RemoveKey("one"));
+
+  // Second removal of previously existing key should fail.
+  EXPECT_FALSE(root.RemoveKey("one"));
+}
+
+TEST(ValuesTest, RemovePath) {
+  Value root(Value::Type::DICTIONARY);
+  root.SetPath({"one", "two", "three"}, Value(123));
+
+  // Removal of missing key should fail.
+  EXPECT_FALSE(root.RemovePath({"one", "two", "four"}));
+
+  // Removal of existing key should succeed.
+  EXPECT_TRUE(root.RemovePath({"one", "two", "three"}));
+
+  // Second removal of previously existing key should fail.
+  EXPECT_FALSE(root.RemovePath({"one", "two", "three"}));
+
+  // Intermediate empty dictionaries should be cleared.
+  EXPECT_FALSE(root.FindPath({"one"}));
+
+  root.SetPath({"one", "two", "three"}, Value(123));
+  root.SetPath({"one", "two", "four"}, Value(124));
+
+  EXPECT_TRUE(root.RemovePath(std::vector<StringPiece>{"one", "two", "three"}));
+  // Intermediate non-empty dictionaries should be kept.
+  EXPECT_TRUE(root.FindPath({"one"}));
+  EXPECT_TRUE(root.FindPath({"one", "two"}));
+  EXPECT_TRUE(root.FindPath({"one", "two", "four"}));
+}
+
 TEST(ValuesTest, Basic) {
   // Test basic dictionary getting/setting
   DictionaryValue settings;