Introduce mutable Value::FindStringKey

This CL introduces a Value::FindStringKey implementation that returns a mutable
string which can be modified inline. This reduces the number of temporary
copies needed for the LogBuffer.

Bug: 928595
Change-Id: Ic09b0bd029805b983a888b691420a9d3b32f4ee3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1724516
Commit-Queue: Dominic Battré <battre@chromium.org>
Auto-Submit: Dominic Battré <battre@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682580}

base/values.cc

@@ -320,6 +320,11 @@ const std::string& Value::GetString() const {
   return string_value_;
 }
 
+std::string& Value::GetString() {
+  CHECK(is_string());
+  return string_value_;
+}
+
 const Value::BlobStorage& Value::GetBlob() const {
   CHECK(is_blob());
   return binary_value_;
@@ -386,6 +391,11 @@ const std::string* Value::FindStringKey(StringPiece key) const {
   return result ? &result->string_value_ : nullptr;
 }
 
+std::string* Value::FindStringKey(StringPiece key) {
+  Value* result = FindKeyOfType(key, Type::STRING);
+  return result ? &result->string_value_ : nullptr;
+}
+
 const Value::BlobStorage* Value::FindBlobKey(StringPiece key) const {
   const Value* value = FindKeyOfType(key, Type::BINARY);
   return value ? &value->binary_value_ : nullptr;
@@ -526,6 +536,11 @@ const std::string* Value::FindStringPath(StringPiece path) const {
   return &cur->string_value_;
 }
 
+std::string* Value::FindStringPath(StringPiece path) {
+  return const_cast<std::string*>(
+      static_cast<const Value*>(this)->FindStringPath(path));
+}
+
 const Value::BlobStorage* Value::FindBlobPath(StringPiece path) const {
   const Value* cur = FindPath(path);
   if (!cur || !cur->is_blob())

base/values.h

@@ -172,6 +172,7 @@ class BASE_EXPORT Value {
   int GetInt() const;
   double GetDouble() const;  // Implicitly converts from int if necessary.
   const std::string& GetString() const;
+  std::string& GetString();
   const BlobStorage& GetBlob() const;
 
   ListStorage& GetList();
@@ -211,6 +212,7 @@ class BASE_EXPORT Value {
 
   // |FindStringKey| returns |nullptr| if value is not found or not a string.
   const std::string* FindStringKey(StringPiece key) const;
+  std::string* FindStringKey(StringPiece key);
 
   // Returns nullptr is value is not found or not a binary.
   const BlobStorage* FindBlobKey(StringPiece key) const;
@@ -318,6 +320,7 @@ class BASE_EXPORT Value {
   base::Optional<int> FindIntPath(StringPiece path) const;
   base::Optional<double> FindDoublePath(StringPiece path) const;
   const std::string* FindStringPath(StringPiece path) const;
+  std::string* FindStringPath(StringPiece path);
   const BlobStorage* FindBlobPath(StringPiece path) const;
   Value* FindDictPath(StringPiece path);
   const Value* FindDictPath(StringPiece path) const;

base/values_unittest.cc

@@ -748,6 +748,20 @@ TEST(ValuesTest, FindStringKey) {
   EXPECT_EQ(nullptr, dict.FindStringKey("dict"));
 }
 
+TEST(ValuesTest, MutableFindStringKey) {
+  Value::DictStorage storage;
+  storage.emplace("string", std::make_unique<Value>("foo"));
+  Value dict(std::move(storage));
+
+  *(dict.FindStringKey("string")) = "bar";
+
+  Value::DictStorage expected_storage;
+  expected_storage.emplace("string", std::make_unique<Value>("bar"));
+  Value expected_dict(std::move(expected_storage));
+
+  EXPECT_EQ(expected_dict, dict);
+}
+
 TEST(ValuesTest, FindDictKey) {
   Value::DictStorage storage;
   storage.emplace("null", std::make_unique<Value>(Value::Type::NONE));
@@ -2374,4 +2388,22 @@ TEST(ValuesTest, FromToUniquePtrValue) {
   EXPECT_EQ(dict_copy, *val);
 }
 
+TEST(ValuesTest, MutableFindStringPath) {
+  Value dict(Value::Type::DICTIONARY);
+  dict.SetStringPath("foo.bar", "value");
+
+  *(dict.FindStringPath("foo.bar")) = "new_value";
+
+  Value expected_dict(Value::Type::DICTIONARY);
+  expected_dict.SetStringPath("foo.bar", "new_value");
+
+  EXPECT_EQ(expected_dict, dict);
+}
+
+TEST(ValuesTest, MutableGetString) {
+  Value value("value");
+  value.GetString() = "new_value";
+  EXPECT_EQ("new_value", value.GetString());
+}
+
 }  // namespace base

components/autofill/core/browser/logging/log_buffer.cc

@@ -55,9 +55,6 @@ void AppendChildToLastNode(std::vector<base::Value>* buffer,
 // and the lengths of strings should be relatively small and we reduce the
 // memory consumption of the DOM, which may grow rather large.
 //
-// TODO(crbug.com/928595) Provide a FindStringKey that returns a mutable string
-// and append to that string.
-//
 // If the last child of the element in buffer is a text node, append |text| to
 // it and return true (successful coalescing). Otherwise return false.
 bool TryCoalesceString(std::vector<base::Value>* buffer,
@@ -72,8 +69,8 @@ bool TryCoalesceString(std::vector<base::Value>* buffer,
   auto& last_child = children->GetList().back();
   if (!IsTextNode(last_child))
     return false;
-  const std::string* old_text = last_child.FindStringKey("value");
-  last_child.SetStringKey("value", base::StrCat({*old_text, text}));
+  std::string* old_text = last_child.FindStringKey("value");
+  old_text->append(text.data(), text.size());
   return true;
 }