Introduce CertNodeBuilder in CertificateViewerDialogHandler::RequestCertificateFields

chrome/browser/ui/webui/certificate_viewer_webui.cc

Index: chrome/browser/ui/webui/certificate_viewer_webui.cc
diff --git a/chrome/browser/ui/webui/certificate_viewer_webui.cc b/chrome/browser/ui/webui/certificate_viewer_webui.cc
index eebe9f1998f233476586b1521c4c6ed7cf46a842..ea9acf6e55058838697745b77ba24e7e07a3cb7f 100644
--- a/chrome/browser/ui/webui/certificate_viewer_webui.cc
+++ b/chrome/browser/ui/webui/certificate_viewer_webui.cc
@@ -11,9 +11,12 @@
 #include "base/bind_helpers.h"
 #include "base/i18n/time_formatting.h"
 #include "base/json/json_writer.h"
+#include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/string_piece.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/values.h"
 #include "chrome/browser/certificate_viewer.h"
 #include "chrome/browser/platform_util.h"
 #include "chrome/browser/ui/browser_dialogs.h"
@@ -30,6 +33,85 @@
 using content::WebContents;
 using content::WebUIMessageHandler;
 
+namespace {
+
+// Helper class for building a Value representation of a certificate. The class
+// gathers data for a single node of the representation tree and builds a
+// DictionaryValue out of that.
+class CertNodeBuilder {
+ public:
+  // Starts the node with "label" set to |label|.
+  explicit CertNodeBuilder(base::StringPiece label);
+
+  // Convenience version: Converts |label_id| to the corresponding resource
+  // string, then delegates to the other constructor.
+  explicit CertNodeBuilder(int label_id);
+
+  // Builder methods all return |*this| so that they can be chained in single
+  // expressions.
+
+  // Sets the "payload.val" field. Call this at most once.
+  CertNodeBuilder& Payload(base::StringPiece payload);
+
+  // Adds |child| in the list keyed "children". Can be called multiple times.
+  CertNodeBuilder& Child(std::unique_ptr<base::DictionaryValue> child);
+
+  // Similar to Child, but if the argument is null, then this does not add
+  // anything.
+  CertNodeBuilder& ChildIfNotNull(std::unique_ptr<base::DictionaryValue> child);
+
+  // Creates a DictionaryValue representation of the collected information. Only
+  // call this once.
+  std::unique_ptr<base::DictionaryValue> Build();
+
+ private:
+  base::DictionaryValue node_;
+  base::ListValue children_;
+  // |built_| is false until Build() is called. Once it is |true|, |node_| and
+  // |children_| are no longer valid for use.
+  bool built_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(CertNodeBuilder);
+};
+
+CertNodeBuilder::CertNodeBuilder(base::StringPiece label) {
+  node_.SetString("label", label);
+}
+
+CertNodeBuilder::CertNodeBuilder(int label_id)
+    : CertNodeBuilder(l10n_util::GetStringUTF8(label_id)) {}
+
+CertNodeBuilder& CertNodeBuilder::Payload(base::StringPiece payload) {
+  DCHECK(!node_.HasKey("payload.val"));
+  node_.SetString("payload.val", payload);
+  return *this;
+}
+
+CertNodeBuilder& CertNodeBuilder::Child(
+    std::unique_ptr<base::DictionaryValue> child) {
+  children_.Append(std::move(child));
+  return *this;
+}
+
+CertNodeBuilder& CertNodeBuilder::ChildIfNotNull(
+    std::unique_ptr<base::DictionaryValue> child) {
+  if (child)
+    return Child(std::move(child));
+  return *this;
+}
+
+std::unique_ptr<base::DictionaryValue> CertNodeBuilder::Build() {
+  DCHECK(!built_);
+  if (!children_.empty()) {
+    node_.Set("children", base::MakeUnique<base::Value>(std::move(children_)));
+  }
+  built_ = true;
+  return base::DictionaryValue::From(
+      base::MakeUnique<base::Value>(std::move(node_)));

[michaelpg, 2017/04/13 01:07:00]

jdoerrie, correct me if I'm wrong, but could this be simplified? e.g.:
    return base::MakeUnique<base::DictionaryValue>(std::move(node_));

Otherwise, it:
  * move-constructs a Value from node_
  * creates a temporary unique_ptr pointing to the new Value
  * calls From() with the temporary unique_ptr
    * verifies the unique_ptr points to a dictionary (it does)
    * copies unique_ptr's pointer and releases the unique_ptr
    * creates and returns a unique_ptr pointing to the copied pointer

I think From() is intended for converting a unique_ptr of Value type to
(possible) DictionaryValue type, but we know what |node_| is.

[vabr (Chromium), 2017/04/13 06:20:26]

Thanks for catching this! You are correct, I somehow missed that |node_| is
already a DictionaryValue.

[jdoerrie, 2017/04/13 08:38:42]

Yes, you are right michaelpg@. Vaclav's updated changelist with dropping From is
the way to go.

+}
+
+}  // namespace
+
 // Shows a certificate using the WebUI certificate viewer.
 void ShowCertificateViewer(WebContents* web_contents,
                            gfx::NativeWindow parent,
@@ -169,7 +251,7 @@ std::string CertificateViewerModalDialog::GetDialogArgs() const {
       x509_certificate_model::HashCertSHA1(cert_hnd));
 
   // Certificate hierarchy is constructed from bottom up.
-  base::ListValue* children = NULL;
+  std::unique_ptr<base::ListValue> children;
   int index = 0;
   for (net::X509Certificate::OSCertHandles::const_iterator i =
       cert_chain.begin(); i != cert_chain.end(); ++i, ++index) {
@@ -180,14 +262,14 @@ std::string CertificateViewerModalDialog::GetDialogArgs() const {
     cert_node->SetDouble("payload.index", index);
     // Add the child from the previous iteration.
     if (children)
-      cert_node->Set("children", children);
+      cert_node->Set("children", std::move(children));
 
     // Add this node to the children list for the next iteration.
-    children = new base::ListValue();
+    children = base::MakeUnique<base::ListValue>();
     children->Append(std::move(cert_node));
   }
   // Set the last node as the top of the certificate hierarchy.
-  cert_info.Set("hierarchy", children);
+  cert_info.Set("hierarchy", std::move(children));
 
   base::JSONWriter::Write(cert_info, &data);
 
@@ -291,181 +373,115 @@ void CertificateViewerDialogHandler::RequestCertificateFields(
 
   net::X509Certificate::OSCertHandle cert = cert_chain_[cert_index];
 
-  // Main certificate fields.
-  auto cert_fields = base::MakeUnique<base::ListValue>();
-  auto node_details = base::MakeUnique<base::DictionaryValue>();
-  node_details->SetString("label",
-      l10n_util::GetStringUTF8(IDS_CERT_DETAILS_VERSION));
+  CertNodeBuilder version_node(IDS_CERT_DETAILS_VERSION);
   std::string version = x509_certificate_model::GetVersion(cert);
   if (!version.empty()) {
-    node_details->SetString("payload.val",
-        l10n_util::GetStringFUTF8(IDS_CERT_DETAILS_VERSION_FORMAT,
-                                  base::UTF8ToUTF16(version)));
+    version_node.Payload(l10n_util::GetStringFUTF8(
+        IDS_CERT_DETAILS_VERSION_FORMAT, base::UTF8ToUTF16(version)));
   }
-  cert_fields->Append(std::move(node_details));
-
-  node_details = base::MakeUnique<base::DictionaryValue>();
-  node_details->SetString("label",
-      l10n_util::GetStringUTF8(IDS_CERT_DETAILS_SERIAL_NUMBER));
-  node_details->SetString("payload.val",
-      x509_certificate_model::GetSerialNumberHexified(cert,
-          l10n_util::GetStringUTF8(IDS_CERT_INFO_FIELD_NOT_PRESENT)));
-  cert_fields->Append(std::move(node_details));
-
-  node_details = base::MakeUnique<base::DictionaryValue>();
-  node_details->SetString("label",
-      l10n_util::GetStringUTF8(IDS_CERT_DETAILS_CERTIFICATE_SIG_ALG));
-  node_details->SetString("payload.val",
-      x509_certificate_model::ProcessSecAlgorithmSignature(cert));
-  cert_fields->Append(std::move(node_details));
-
-  node_details = base::MakeUnique<base::DictionaryValue>();
-  node_details->SetString("label",
-      l10n_util::GetStringUTF8(IDS_CERT_DETAILS_ISSUER));
-  node_details->SetString("payload.val",
-      x509_certificate_model::GetIssuerName(cert));
-  cert_fields->Append(std::move(node_details));
-
-  // Validity period.
-  auto cert_sub_fields = base::MakeUnique<base::ListValue>();
-
-  auto sub_node_details = base::MakeUnique<base::DictionaryValue>();
-  sub_node_details->SetString(
-      "label", l10n_util::GetStringUTF8(IDS_CERT_DETAILS_NOT_BEFORE));
-
-  auto alt_node_details = base::MakeUnique<base::DictionaryValue>();
-  alt_node_details->SetString("label",
-      l10n_util::GetStringUTF8(IDS_CERT_DETAILS_NOT_AFTER));
 
+  CertNodeBuilder issued_node_builder(IDS_CERT_DETAILS_NOT_BEFORE);
+  CertNodeBuilder expires_node_builder(IDS_CERT_DETAILS_NOT_AFTER);
   base::Time issued, expires;
   if (x509_certificate_model::GetTimes(cert, &issued, &expires)) {
-    sub_node_details->SetString(
-        "payload.val",
-        base::UTF16ToUTF8(
-            base::TimeFormatShortDateAndTimeWithTimeZone(issued)));
-    alt_node_details->SetString(
-        "payload.val",
-        base::UTF16ToUTF8(
-            base::TimeFormatShortDateAndTimeWithTimeZone(expires)));
+    issued_node_builder.Payload(base::UTF16ToUTF8(
+        base::TimeFormatShortDateAndTimeWithTimeZone(issued)));
+    expires_node_builder.Payload(base::UTF16ToUTF8(
+        base::TimeFormatShortDateAndTimeWithTimeZone(expires)));
   }
-  cert_sub_fields->Append(std::move(sub_node_details));
-  cert_sub_fields->Append(std::move(alt_node_details));
-
-  node_details = base::MakeUnique<base::DictionaryValue>();
-  node_details->SetString("label",
-                          l10n_util::GetStringUTF8(IDS_CERT_DETAILS_VALIDITY));
-  node_details->Set("children", std::move(cert_sub_fields));
-  cert_fields->Append(std::move(node_details));
-
-  node_details = base::MakeUnique<base::DictionaryValue>();
-  node_details->SetString("label",
-      l10n_util::GetStringUTF8(IDS_CERT_DETAILS_SUBJECT));
-  node_details->SetString("payload.val",
-      x509_certificate_model::GetSubjectName(cert));
-  cert_fields->Append(std::move(node_details));
-
-  // Subject key information.
-  cert_sub_fields = base::MakeUnique<base::ListValue>();
-
-  sub_node_details = base::MakeUnique<base::DictionaryValue>();
-  sub_node_details->SetString(
-      "label", l10n_util::GetStringUTF8(IDS_CERT_DETAILS_SUBJECT_KEY_ALG));
-  sub_node_details->SetString(
-      "payload.val",
-      x509_certificate_model::ProcessSecAlgorithmSubjectPublicKey(cert));
-  cert_sub_fields->Append(std::move(sub_node_details));
-
-  sub_node_details = base::MakeUnique<base::DictionaryValue>();
-  sub_node_details->SetString(
-      "label", l10n_util::GetStringUTF8(IDS_CERT_DETAILS_SUBJECT_KEY));
-  sub_node_details->SetString(
-      "payload.val", x509_certificate_model::ProcessSubjectPublicKeyInfo(cert));
-  cert_sub_fields->Append(std::move(sub_node_details));
-
-  node_details = base::MakeUnique<base::DictionaryValue>();
-  node_details->SetString(
-      "label", l10n_util::GetStringUTF8(IDS_CERT_DETAILS_SUBJECT_KEY_INFO));
-  node_details->Set("children", std::move(cert_sub_fields));
-  cert_fields->Append(std::move(node_details));
-
-  // Extensions.
+
   x509_certificate_model::Extensions extensions;
   x509_certificate_model::GetExtensions(
       l10n_util::GetStringUTF8(IDS_CERT_EXTENSION_CRITICAL),
       l10n_util::GetStringUTF8(IDS_CERT_EXTENSION_NON_CRITICAL),
       cert, &extensions);
 
+  std::unique_ptr<base::DictionaryValue> details_extensions;
   if (!extensions.empty()) {
-    cert_sub_fields = base::MakeUnique<base::ListValue>();
-
-    for (x509_certificate_model::Extensions::const_iterator i =
-         extensions.begin(); i != extensions.end(); ++i) {
-      sub_node_details = base::MakeUnique<base::DictionaryValue>();
-      sub_node_details->SetString("label", i->name);
-      sub_node_details->SetString("payload.val", i->value);
-      cert_sub_fields->Append(std::move(sub_node_details));
+    CertNodeBuilder details_extensions_builder(IDS_CERT_DETAILS_EXTENSIONS);
+    for (const x509_certificate_model::Extension& extension : extensions) {
+      details_extensions_builder.Child(
+          CertNodeBuilder(extension.name).Payload(extension.value).Build());
     }
-
-    node_details = base::MakeUnique<base::DictionaryValue>();
-    node_details->SetString(
-        "label", l10n_util::GetStringUTF8(IDS_CERT_DETAILS_EXTENSIONS));
-    node_details->Set("children", std::move(cert_sub_fields));
-    cert_fields->Append(std::move(node_details));
+    details_extensions = details_extensions_builder.Build();
   }
 
-  // Details certificate information.
-  node_details = base::MakeUnique<base::DictionaryValue>();
-  node_details->SetString("label",
-      l10n_util::GetStringUTF8(IDS_CERT_DETAILS_CERTIFICATE_SIG_ALG));
-  node_details->SetString("payload.val",
-      x509_certificate_model::ProcessSecAlgorithmSignatureWrap(cert));
-  cert_fields->Append(std::move(node_details));
-
-  node_details = base::MakeUnique<base::DictionaryValue>();
-  node_details->SetString("label",
-      l10n_util::GetStringUTF8(IDS_CERT_DETAILS_CERTIFICATE_SIG_VALUE));
-  node_details->SetString("payload.val",
-      x509_certificate_model::ProcessRawBitsSignatureWrap(cert));
-  cert_fields->Append(std::move(node_details));
-
-  // Fingerprint information.
-  cert_sub_fields = base::MakeUnique<base::ListValue>();
-
-  sub_node_details = base::MakeUnique<base::DictionaryValue>();
-  sub_node_details->SetString(
-      "label",
-      l10n_util::GetStringUTF8(IDS_CERT_INFO_SHA256_FINGERPRINT_LABEL));
-  sub_node_details->SetString("payload.val",
-                              x509_certificate_model::HashCertSHA256(cert));
-  cert_sub_fields->Append(std::move(sub_node_details));
-
-  sub_node_details = base::MakeUnique<base::DictionaryValue>();
-  sub_node_details->SetString(
-      "label", l10n_util::GetStringUTF8(IDS_CERT_INFO_SHA1_FINGERPRINT_LABEL));
-  sub_node_details->SetString("payload.val",
-                              x509_certificate_model::HashCertSHA1(cert));
-  cert_sub_fields->Append(std::move(sub_node_details));
-
-  node_details = base::MakeUnique<base::DictionaryValue>();
-  node_details->SetString(
-      "label", l10n_util::GetStringUTF8(IDS_CERT_INFO_FINGERPRINTS_GROUP));
-  node_details->Set("children", std::move(cert_sub_fields));
-  cert_fields->Append(std::move(node_details));
-
-  // Certificate information.
-  node_details = base::MakeUnique<base::DictionaryValue>();
-  node_details->SetString(
-      "label", l10n_util::GetStringUTF8(IDS_CERT_DETAILS_CERTIFICATE));
-  node_details->Set("children", std::move(cert_fields));
-  cert_fields = base::MakeUnique<base::ListValue>();
-  cert_fields->Append(std::move(node_details));
-
-  // Top level information.
   base::ListValue root_list;
-  node_details = base::MakeUnique<base::DictionaryValue>();
-  node_details->SetString("label", x509_certificate_model::GetTitle(cert));
-  node_details->Set("children", std::move(cert_fields));
-  root_list.Append(std::move(node_details));
+  root_list.Append(
+      CertNodeBuilder(x509_certificate_model::GetTitle(cert))
+          .Child(
+              CertNodeBuilder(
+                  l10n_util::GetStringUTF8(IDS_CERT_DETAILS_CERTIFICATE))
+                  // Main certificate fields.
+                  .Child(version_node.Build())
+                  .Child(
+                      CertNodeBuilder(IDS_CERT_DETAILS_SERIAL_NUMBER)
+                          .Payload(
+                              x509_certificate_model::GetSerialNumberHexified(
+                                  cert, l10n_util::GetStringUTF8(
+                                            IDS_CERT_INFO_FIELD_NOT_PRESENT)))
+                          .Build())
+                  .Child(CertNodeBuilder(IDS_CERT_DETAILS_CERTIFICATE_SIG_ALG)
+                             .Payload(x509_certificate_model::
+                                          ProcessSecAlgorithmSignature(cert))
+                             .Build())
+                  .Child(
+                      CertNodeBuilder(IDS_CERT_DETAILS_ISSUER)
+                          .Payload(x509_certificate_model::GetIssuerName(cert))
+                          .Build())
+                  // Validity period.
+                  .Child(CertNodeBuilder(IDS_CERT_DETAILS_VALIDITY)
+                             .Child(issued_node_builder.Build())
+                             .Child(expires_node_builder.Build())
+                             .Build())
+                  .Child(
+                      CertNodeBuilder(IDS_CERT_DETAILS_SUBJECT)
+                          .Payload(x509_certificate_model::GetSubjectName(cert))
+                          .Build())
+                  // Subject key information.
+                  .Child(
+                      CertNodeBuilder(IDS_CERT_DETAILS_SUBJECT_KEY_INFO)
+                          .Child(
+                              CertNodeBuilder(IDS_CERT_DETAILS_SUBJECT_KEY_ALG)
+                                  .Payload(
+                                      x509_certificate_model::
+                                          ProcessSecAlgorithmSubjectPublicKey(
+                                              cert))
+                                  .Build())
+                          .Child(CertNodeBuilder(IDS_CERT_DETAILS_SUBJECT_KEY)
+                                     .Payload(
+                                         x509_certificate_model::
+                                             ProcessSubjectPublicKeyInfo(cert))
+                                     .Build())
+                          .Build())
+                  // Extensions.
+                  .ChildIfNotNull(std::move(details_extensions))
+                  .Child(
+                      CertNodeBuilder(IDS_CERT_DETAILS_CERTIFICATE_SIG_ALG)
+                          .Payload(x509_certificate_model::
+                                       ProcessSecAlgorithmSignatureWrap(cert))
+                          .Build())
+                  .Child(CertNodeBuilder(IDS_CERT_DETAILS_CERTIFICATE_SIG_VALUE)
+                             .Payload(x509_certificate_model::
+                                          ProcessRawBitsSignatureWrap(cert))
+                             .Build())
+                  .Child(
+                      CertNodeBuilder(IDS_CERT_INFO_FINGERPRINTS_GROUP)
+                          .Child(CertNodeBuilder(
+                                     IDS_CERT_INFO_SHA256_FINGERPRINT_LABEL)
+                                     .Payload(
+                                         x509_certificate_model::HashCertSHA256(
+                                             cert))
+                                     .Build())
+                          .Child(
+                              CertNodeBuilder(
+                                  IDS_CERT_INFO_SHA1_FINGERPRINT_LABEL)
+                                  .Payload(x509_certificate_model::HashCertSHA1(
+                                      cert))
+                                  .Build())
+                          .Build())
+                  .Build())
+          .Build());

[michaelpg, 2017/04/13 01:07:00]

If you like, we could remove all these .Build() boilerplate lines by adding
    CertNodeBuilder& CertNodeBuilder::Child(CertNodeBuilder&)

that way, .Build() would only be needed when you need to use the DictionaryValue
directly.

[vabr (Chromium), 2017/04/14 07:59:04]

I like this, but the style guide forbids passing non-const references. Changing
that to pointer arguments won't work either, because the compiler wisely advises
against taking the address of a temporary.

I could have Child accept CertNodeBuilder simply by value, but then the result
of the passed expression would need to be an rvalue. That's the case for
CertNodeBuilder(...) but not for CertNodeBuilder(...).Payload(...) etc., because
Payload and Child return a reference, not a plain value. And they cannot return
the plain value (something like return std::move(*this);) because that would
break in case they are called on a named instance of a CertNodeBuilder (CNB
builder; builder.Payload(...); would destroy the builder).

I am not sure how to get rid of the intermediate Build() without adding more
boilerplate elsewhere, or without violating the style guide. So I propose to
keep the current code. What do you think?

[michaelpg, 2017/04/14 18:08:31]

yeah, any way I can think of would violate some style guide rule or another. I
guess that's by design.

 
   // Send certificate information to javascript.
   web_ui()->CallJavascriptFunctionUnsafe("cert_viewer.getCertificateFields",