Introduce SerializeDictionaryForest for devtools_target_ui

chrome/browser/devtools/serialize_host_descriptions.cc

Index: chrome/browser/devtools/serialize_host_descriptions.cc
diff --git a/chrome/browser/devtools/serialize_host_descriptions.cc b/chrome/browser/devtools/serialize_host_descriptions.cc
new file mode 100644
index 0000000000000000000000000000000000000000..9e48a82378e6c14fa295e81d06a65b21fe930bf0
--- /dev/null
+++ b/chrome/browser/devtools/serialize_host_descriptions.cc
@@ -0,0 +1,103 @@
+// Copyright 2017 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/devtools/serialize_host_descriptions.h"
+
+#include <map>
+#include <tuple>
+#include <utility>
+
+#include "base/memory/ptr_util.h"
+#include "base/strings/string_piece.h"
+
+namespace {
+
+// Returns the serialization of |root|. It expects |children[x]| to be the
+// vector of child nodes for all descendants |x| of |root|. The serialization
+// consists of taking the |representation| value of each node, starting in
+// leaves, and injecting children's representations into a ListValue under the
+// key |child_key| in the parent's |representation|. This is desctructive to the
+// representation stored with the nodes (which gets moved out of them).
+base::DictionaryValue Serialize(
+    base::StringPiece child_key,
+    base::DictionaryValue* root,
+    const std::map<base::DictionaryValue*, std::vector<base::DictionaryValue*>>&
+        children) {
+  auto children_list = base::MakeUnique<base::ListValue>();
+  auto child_it = children.find(root);
+  if (child_it != children.end()) {
+    for (base::DictionaryValue* child : child_it->second) {
+      children_list->base::Value::GetList().push_back(
+          Serialize(child_key, child, children));
+    }
+  }
+
+  if (!children_list->empty())
+    root->Set(child_key, std::move(children_list));
+  return std::move(*root);
+}
+
+// Takes a vector of host description and converts it into a tuple of:
+// (1) map from a host's representation to representations of its children.
+// (2) a vector of representations of hosts with no parents,
+// (3) a vector actually storing all those representations to which the rest
+//     just points.
+std::tuple<
+    std::map<base::DictionaryValue*, std::vector<base::DictionaryValue*>>,
+    std::vector<base::DictionaryValue*>,
+    std::vector<base::DictionaryValue>>
+CreateDictionaryForest(std::vector<HostDescriptionNode> hosts) {
+  std::map<base::DictionaryValue*, std::vector<base::DictionaryValue*>>
+      children;
+  std::vector<base::DictionaryValue*> roots;
+  std::vector<base::DictionaryValue> representations;
+  representations.reserve(hosts.size());
+
+  std::map<base::StringPiece, base::DictionaryValue*> name_to_representation;
+
+  // First move the representations and map the names to them.
+  for (HostDescriptionNode& node : hosts) {
+    representations.push_back(std::move(node.representation));
+    name_to_representation[node.name] = &representations.back();
+  }
+
+  // Now compute children.
+  for (HostDescriptionNode& node : hosts) {
+    base::DictionaryValue* node_rep = name_to_representation[node.name];
+    base::StringPiece parent_name = node.parent_name;
+    if (parent_name.empty()) {
+      roots.push_back(node_rep);
+      continue;
+    }
+    auto node_it = name_to_representation.find(parent_name);
+    if (node_it == name_to_representation.end()) {
+      roots.push_back(node_rep);
+      continue;
+    }
+    children[name_to_representation[parent_name]].push_back(node_rep);
+  }
+
+  return std::make_tuple(std::move(children), std::move(roots),
+                         std::move(representations));
+}
+
+}  // namespace
+
+base::ListValue SerializeHostDescriptions(
+    std::vector<HostDescriptionNode> hosts,
+    base::StringPiece child_key) {
+  std::map<base::DictionaryValue*, std::vector<base::DictionaryValue*>>
+      children;
+  std::vector<base::DictionaryValue*> roots;
+  std::vector<base::DictionaryValue> representations;
+  std::tie(children, roots, representations) =
+      CreateDictionaryForest(std::move(hosts));

[dgozman, 2017/04/19 22:05:14]

It's usually less typing to use output params rather than return a tuple.

[vabr (Chromium), 2017/04/20 07:29:39]

I thought about that, but even with output params the map and three vectors
looked pretty horrible, and on the callsite it's roughly the same.

I considered three approaches here:
(1) output params
  Pros: only mentioning the types once (instead of twice) in
        CreateDictionaryForest; also no need to explain what parts of the
        tuple do what in a comment
  Cons: the need to .clear() the arguments on call (or have long comments
        about contracts)
(2) returning a tuple
  Pros: clear semantics of a return typ (no .clear(), no need to point out this
        is output param, etc.)
  Cons: repeating the types twice in CreateDictionaryForest
(3) encapsulating in a class/struct
  Pros: one-liner on the callsite
  Cons: too much boilerplate, the most verbose, and also having a class with
        no real methods apart from a heavy constructor is strange


To me, (3) seemed like the worst solution, and I liked the clear concept
involved in (2), which is what I favoured.

But I have no strong opposition to (1), and I will to switch to that, given that
you favour it.

+
+  base::ListValue list_value;
+  for (auto* root : roots) {
+    list_value.base::Value::GetList().push_back(

[dgozman, 2017/04/19 22:05:14]

I'm curious why do you have to specify the full name through the base class?

[vabr (Chromium), 2017/04/20 07:29:39]

Currenty, both Value and ListValue define GetList. The former takes 0 arguments
and returns the underlying vector storing things in case the said Value is in
fact a ListValue. That's a new method added as a part of moving to value
semantics for base::Value. The latter is just an old convenience accessor in
ListValue, taking 2 arguments and returning a particular sub-value and
converting it to ListValue.

The compiler fails to find the correct GetList and just complains that calling
GetList on ListValue with no arguments is wrong. So I needed to be explicit
about which GetList I wanted.

This is only temporary. As the refactoring of base::Value progresses, all the
subclasses, including ListValue, will be merged into Value (such as already all
except for ListValue and DictionaryValue were merged). At that point, this
problem will disappear and this code will get the unnecessary base::Value::
specifier deleted.

More details about the base::Value refactoring can be found in
https://crbug.com/646113.

+        Serialize(child_key, root, children));
+  }
+  return list_value;
+}