Introduce concept of fragments

The LogBuffer had some surprising behavior:

// This would DCHECK;
LogBuffer buf1;
buf1 << "Foo" << Tag{"div"};

// This would create two nested divs instead of a sequence of two divs:
LogBuffer buf2;
buf1 << Tag{"div"} << CTag{} << Tag{"div"} << CTag{};

This CL fixes these problems by introducing fragments - the equivalent to the
DOM DocumentFragment. The root of each LogBuffer is now a fragment that can
host an arbitrary number of children of type text and element.

Some optimizations occur: A LogBuffer with a single child in the fragment will
return this child when LogBuffer::RetrieveResult() is called. Similarly,
streaming a LogBuffer into another one will inline the children of the first
into the second instead of appendin the fragment. Due to these optimizations,
the JavaScript handler in the WebUI can be sure that only the outer most Node
is a fragment.

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

components/autofill/core/browser/autofill_and_password_manager_internals/autofill_and_password_manager_internals.js

@@ -47,7 +47,15 @@ function addRawLog(node) {
     return;
   }
   logDiv.appendChild(document.createElement('hr'));
-  logDiv.appendChild(nodeToDomNode(node));
+  if (node.type === 'fragment') {
+    if ('children' in node) {
+      node.children.forEach((child) => {
+        logDiv.appendChild(nodeToDomNode(child));
+      });
+    }
+  } else {
+    logDiv.appendChild(nodeToDomNode(node));
+  }
 }
 
 function setUpAutofillInternals() {

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

@@ -13,11 +13,6 @@ namespace autofill {
 
 namespace {
 
-// The LogBuffer creates a tree that will be rendered as HTML code. This tree
-// supports two node types, "element" (representing a DOM Element) and "text"
-// (representing a text node in the DOM). This function is used to check that
-// attributes and children are only added to "element" nodes but not to "text"
-// nodes.
 bool IsElement(const base::Value& value) {
   const std::string* type = value.FindStringKey("type");
   return type && *type == "element";
@@ -28,6 +23,11 @@ bool IsTextNode(const base::Value& value) {
   return type && *type == "text";
 }
 
+bool IsFragment(const base::Value& value) {
+  const std::string* type = value.FindStringKey("type");
+  return type && *type == "fragment";
+}
+
 void AppendChildToLastNode(std::vector<base::Value>* buffer,
                            base::Value&& new_child) {
   if (buffer->empty()) {
@@ -36,7 +36,8 @@ void AppendChildToLastNode(std::vector<base::Value>* buffer,
   }
 
   base::Value& parent = buffer->back();
-  DCHECK(IsElement(parent));
+  // Elements and Fragments can have children, but TextNodes cannot.
+  DCHECK(!IsTextNode(parent));
 
   if (auto* children = parent.FindListKey("children")) {
     children->GetList().push_back(std::move(new_child));
@@ -76,21 +77,41 @@ bool TryCoalesceString(std::vector<base::Value>* buffer,
   return true;
 }
 
+base::Value CreateEmptyFragment() {
+  base::Value::DictStorage storage;
+  storage.try_emplace("type", std::make_unique<base::Value>("fragment"));
+  return base::Value(storage);
+}
+
 }  // namespace
 
-LogBuffer::LogBuffer() = default;
+LogBuffer::LogBuffer() {
+  buffer_.push_back(CreateEmptyFragment());
+}
+
 LogBuffer::LogBuffer(LogBuffer&& other) noexcept = default;
 LogBuffer::~LogBuffer() = default;
 
 base::Value LogBuffer::RetrieveResult() {
-  if (buffer_.empty())
-    return base::Value();
+  // The buffer should always start with a fragment.
+  DCHECK(buffer_.size() >= 1);
 
   // Close not-yet-closed tags.
   while (buffer_.size() > 1)
     *this << CTag{};
 
-  return std::exchange(buffer_.back(), base::Value());
+  auto* children = buffer_[0].FindListKey("children");
+  if (!children || children->GetList().empty())
+    return base::Value();
+
+  // If the fragment has a single child, return that directly.
+  if (children->GetList().size() == 1) {
+    base::Value result = std::move(children->GetList().back());
+    children->GetList().pop_back();
+    return result;
+  }
+
+  return std::exchange(buffer_.back(), CreateEmptyFragment());
 }
 
 LogBuffer& operator<<(LogBuffer& buf, Tag&& tag) {
@@ -108,8 +129,7 @@ LogBuffer& operator<<(LogBuffer& buf, Tag&& tag) {
 LogBuffer& operator<<(LogBuffer& buf, CTag&& tag) {
   if (!buf.active())
     return buf;
-  // Don't close the very first opened tag. It stays and gets returned in the
-  // end.
+  // Don't close the fragment. It stays and gets returned in the end.
   if (buf.buffer_.size() <= 1)
     return buf;
 
@@ -174,6 +194,15 @@ LogBuffer& operator<<(LogBuffer& buf, LogBuffer&& buffer) {
   base::Value node_to_add(buffer.RetrieveResult());
   if (node_to_add.is_none())
     return buf;
+
+  if (IsFragment(node_to_add)) {
+    auto* children = node_to_add.FindListKey("children");
+    if (!children)
+      return buf;
+    for (auto& child : children->GetList())
+      AppendChildToLastNode(&buf.buffer_, std::exchange(child, base::Value()));
+    return buf;
+  }
   AppendChildToLastNode(&buf.buffer_, std::move(node_to_add));
   return buf;
 }

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

@@ -103,13 +103,14 @@ class LogBuffer {
 
   // The stack of values being constructed. Each item is a dictionary with the
   // following attributes:
-  // - type: 'element' | 'text'
+  // - type: 'element' | 'fragment' | 'text'
   // - value: name of tag | text content
   // - children (opt): list of child nodes
   // - attributes (opt): dictionary of name/value pairs
   // The |buffer_| serves as a stack where the last element is being
   // constructed. Once it is read (i.e. closed via a CTag), it is popped from
   // the stack and attached as a child of the previously second last element.
+  // Only the first element of buffer_ is a 'fragment' and it is never closed.
   std::vector<base::Value> buffer_;
 
   bool active_ = true;

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

@@ -188,4 +188,38 @@ TEST(LogBuffer, LogTableRowBuffer) {
   EXPECT_EQ(expected.RetrieveResult(), actual.RetrieveResult());
 }
 
+TEST(LogBuffer, CreateFragment) {
+  LogBuffer buffer;
+  buffer << "foo" << Br{} << "bar";
+  std::string json;
+  EXPECT_TRUE(base::JSONWriter::Write(buffer.RetrieveResult(), &json));
+  EXPECT_EQ(R"({"children":[{"type":"text","value":"foo"},)"
+            R"({"type":"element","value":"br"},{"type":"text","value":"bar"}],)"
+            R"("type":"fragment"})",
+            json);
+}
+
+TEST(LogBuffer, AppendFragmentByInlining) {
+  LogBuffer tmp_buffer;
+  tmp_buffer << "foo" << Br{} << "bar";
+  LogBuffer buffer;
+  buffer << std::move(tmp_buffer);
+  std::string json;
+  EXPECT_TRUE(base::JSONWriter::Write(buffer.RetrieveResult(), &json));
+  EXPECT_EQ(R"({"children":[{"type":"text","value":"foo"},)"
+            R"({"type":"element","value":"br"},{"type":"text","value":"bar"}],)"
+            R"("type":"fragment"})",
+            json);
+}
+
+TEST(LogBuffer, AppendSingleElementBuffer) {
+  LogBuffer tmp_buffer;
+  tmp_buffer << "foo";
+  LogBuffer buffer;
+  buffer << std::move(tmp_buffer);
+  std::string json;
+  EXPECT_TRUE(base::JSONWriter::Write(buffer.RetrieveResult(), &json));
+  EXPECT_EQ(R"({"type":"text","value":"foo"})", json);
+}
+
 }  // namespace autofill