Change base::Value::ListStorage to std::vector<base::Value>

chrome/browser/ui/webui/certificate_viewer_webui.cc

 // Copyright (c) 2012 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/ui/webui/certificate_viewer_webui.h"
 
 #include <memory>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 273 matching lines @@
 }
 
 void CertificateViewerDialogHandler::RequestCertificateFields(
     const base::ListValue* args) {
   int cert_index = GetCertificateIndex(args);
   if (cert_index < 0)
     return;
 
   net::X509Certificate::OSCertHandle cert = cert_chain_[cert_index];
 
+  // In the following the list.Append(...); list.GetDictionary(...); pattern is
+  // necessary to obtain a raw dict pointer to the last element of the list.
   base::ListValue root_list;
   base::DictionaryValue* node_details;
   base::DictionaryValue* alt_node_details;
   base::ListValue* cert_sub_fields;
-  root_list.Append(
-      base::WrapUnique(node_details = new base::DictionaryValue()));
+  root_list.Append(base::MakeUnique<base::DictionaryValue>());
+  root_list.GetDictionary(root_list.GetSize() - 1, &node_details);

[flackr, 2017/03/15 15:29:39]

I think this would be a lot cleaner and more efficient to defer appending until
we were done constructing each descendant value so that we never need the raw
pointers. i.e.
std::unique_ptr<base::DictionaryValue> node_details =
MakeUnique(base::DictionaryValue>());
node_details->SetString("label", ...);

// construct and append all nodes to node_details
// and finally

root_list.Append(std::move(node_details));

[jdoerrie, 2017/03/23 18:11:17]

Done. This turned out to be quite tricky with the various levels of nesting.

[flackr, 2017/03/23 19:23:57]

I see that, thanks for bearing with me on this. I think the only way to make
things cleaner would be to have more unique_ptrs which are more locally scoped,
then the dictionaries and lists could still be roughly built in top-down order
but appended in bottom-up order. I don't want to put this burden on you though.

   node_details->SetString("label", x509_certificate_model::GetTitle(cert));
 
   base::ListValue* cert_fields;
   node_details->Set("children", cert_fields = new base::ListValue());
-  cert_fields->Append(
-      base::WrapUnique(node_details = new base::DictionaryValue()));
+  cert_fields->Append(base::MakeUnique<base::DictionaryValue>());
+  cert_fields->GetDictionary(cert_fields->GetSize() - 1, &node_details);
 
   node_details->SetString("label",
       l10n_util::GetStringUTF8(IDS_CERT_DETAILS_CERTIFICATE));
   node_details->Set("children", cert_fields = new base::ListValue());
 
   // Main certificate fields.
-  cert_fields->Append(
-      base::WrapUnique(node_details = new base::DictionaryValue()));
+  cert_fields->Append(base::MakeUnique<base::DictionaryValue>());
+  cert_fields->GetDictionary(cert_fields->GetSize() - 1, &node_details);
   node_details->SetString("label",
       l10n_util::GetStringUTF8(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)));
 
-  cert_fields->Append(
-      base::WrapUnique(node_details = new base::DictionaryValue()));
+  cert_fields->Append(base::MakeUnique<base::DictionaryValue>());
+  cert_fields->GetDictionary(cert_fields->GetSize() - 1, &node_details);
   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(
-      base::WrapUnique(node_details = new base::DictionaryValue()));
+  cert_fields->Append(base::MakeUnique<base::DictionaryValue>());
+  cert_fields->GetDictionary(cert_fields->GetSize() - 1, &node_details);
   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(
-      base::WrapUnique(node_details = new base::DictionaryValue()));
+  cert_fields->Append(base::MakeUnique<base::DictionaryValue>());
+  cert_fields->GetDictionary(cert_fields->GetSize() - 1, &node_details);
   node_details->SetString("label",
       l10n_util::GetStringUTF8(IDS_CERT_DETAILS_ISSUER));
   node_details->SetString("payload.val",
       x509_certificate_model::GetIssuerName(cert));
 
   // Validity period.
-  cert_fields->Append(
-      base::WrapUnique(node_details = new base::DictionaryValue()));
+  cert_fields->Append(base::MakeUnique<base::DictionaryValue>());
+  cert_fields->GetDictionary(cert_fields->GetSize() - 1, &node_details);
   node_details->SetString("label",
       l10n_util::GetStringUTF8(IDS_CERT_DETAILS_VALIDITY));
 
   node_details->Set("children", cert_sub_fields = new base::ListValue());
-  cert_sub_fields->Append(
-      base::WrapUnique(node_details = new base::DictionaryValue()));
+  cert_sub_fields->Append(base::MakeUnique<base::DictionaryValue>());
+  cert_sub_fields->GetDictionary(cert_sub_fields->GetSize() - 1, &node_details);
   node_details->SetString("label",
       l10n_util::GetStringUTF8(IDS_CERT_DETAILS_NOT_BEFORE));
-  cert_sub_fields->Append(
-      base::WrapUnique(alt_node_details = new base::DictionaryValue()));
+  cert_sub_fields->Append(base::MakeUnique<base::DictionaryValue>());
+  // |Append| might have invalidated |node_details|, which is why it needs to be
+  // queried again.
+  cert_sub_fields->GetDictionary(cert_sub_fields->GetSize() - 2, &node_details);
+  cert_sub_fields->GetDictionary(cert_sub_fields->GetSize() - 1,
+                                 &alt_node_details);
   alt_node_details->SetString("label",
       l10n_util::GetStringUTF8(IDS_CERT_DETAILS_NOT_AFTER));
   base::Time issued, expires;
   if (x509_certificate_model::GetTimes(cert, &issued, &expires)) {
     node_details->SetString(
         "payload.val",
         base::UTF16ToUTF8(
             base::TimeFormatShortDateAndTimeWithTimeZone(issued)));
     alt_node_details->SetString(
         "payload.val",
         base::UTF16ToUTF8(
             base::TimeFormatShortDateAndTimeWithTimeZone(expires)));
   }
 
-  cert_fields->Append(
-      base::WrapUnique(node_details = new base::DictionaryValue()));
+  cert_fields->Append(base::MakeUnique<base::DictionaryValue>());
+  cert_fields->GetDictionary(cert_fields->GetSize() - 1, &node_details);
   node_details->SetString("label",
       l10n_util::GetStringUTF8(IDS_CERT_DETAILS_SUBJECT));
   node_details->SetString("payload.val",
       x509_certificate_model::GetSubjectName(cert));
 
   // Subject key information.
-  cert_fields->Append(
-      base::WrapUnique(node_details = new base::DictionaryValue()));
+  cert_fields->Append(base::MakeUnique<base::DictionaryValue>());
+  cert_fields->GetDictionary(cert_fields->GetSize() - 1, &node_details);
   node_details->SetString("label",
       l10n_util::GetStringUTF8(IDS_CERT_DETAILS_SUBJECT_KEY_INFO));
 
   node_details->Set("children", cert_sub_fields = new base::ListValue());
-  cert_sub_fields->Append(
-      base::WrapUnique(node_details = new base::DictionaryValue()));
+  cert_sub_fields->Append(base::MakeUnique<base::DictionaryValue>());
+  cert_sub_fields->GetDictionary(cert_sub_fields->GetSize() - 1, &node_details);
   node_details->SetString("label",
       l10n_util::GetStringUTF8(IDS_CERT_DETAILS_SUBJECT_KEY_ALG));
   node_details->SetString("payload.val",
       x509_certificate_model::ProcessSecAlgorithmSubjectPublicKey(cert));
-  cert_sub_fields->Append(
-      base::WrapUnique(node_details = new base::DictionaryValue()));
+  cert_sub_fields->Append(base::MakeUnique<base::DictionaryValue>());
+  cert_sub_fields->GetDictionary(cert_sub_fields->GetSize() - 1, &node_details);
   node_details->SetString("label",
       l10n_util::GetStringUTF8(IDS_CERT_DETAILS_SUBJECT_KEY));
   node_details->SetString("payload.val",
       x509_certificate_model::ProcessSubjectPublicKeyInfo(cert));
 
   // 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);
 
   if (!extensions.empty()) {
-    cert_fields->Append(
-        base::WrapUnique(node_details = new base::DictionaryValue()));
+    cert_fields->Append(base::MakeUnique<base::DictionaryValue>());
+    cert_fields->GetDictionary(cert_fields->GetSize() - 1, &node_details);
     node_details->SetString("label",
         l10n_util::GetStringUTF8(IDS_CERT_DETAILS_EXTENSIONS));
 
     node_details->Set("children", cert_sub_fields = new base::ListValue());
     for (x509_certificate_model::Extensions::const_iterator i =
          extensions.begin(); i != extensions.end(); ++i) {
-      cert_sub_fields->Append(
-          base::WrapUnique(node_details = new base::DictionaryValue()));
+      cert_sub_fields->Append(base::MakeUnique<base::DictionaryValue>());
+      cert_sub_fields->GetDictionary(cert_fields->GetSize() - 1, &node_details);
       node_details->SetString("label", i->name);
       node_details->SetString("payload.val", i->value);
     }
   }
 
-  cert_fields->Append(
-      base::WrapUnique(node_details = new base::DictionaryValue()));
+  cert_fields->Append(base::MakeUnique<base::DictionaryValue>());
+  cert_fields->GetDictionary(cert_fields->GetSize() - 1, &node_details);
   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(
-      base::WrapUnique(node_details = new base::DictionaryValue()));
+  cert_fields->Append(base::MakeUnique<base::DictionaryValue>());
+  cert_fields->GetDictionary(cert_fields->GetSize() - 1, &node_details);
   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(
-      base::WrapUnique(node_details = new base::DictionaryValue()));
+  cert_fields->Append(base::MakeUnique<base::DictionaryValue>());
+  cert_fields->GetDictionary(cert_fields->GetSize() - 1, &node_details);
   node_details->SetString("label",
       l10n_util::GetStringUTF8(IDS_CERT_INFO_FINGERPRINTS_GROUP));
   node_details->Set("children", cert_sub_fields = new base::ListValue());
 
-  cert_sub_fields->Append(
-      base::WrapUnique(node_details = new base::DictionaryValue()));
+  cert_sub_fields->Append(base::MakeUnique<base::DictionaryValue>());
+  cert_sub_fields->GetDictionary(cert_sub_fields->GetSize() - 1, &node_details);
   node_details->SetString("label",
       l10n_util::GetStringUTF8(IDS_CERT_INFO_SHA256_FINGERPRINT_LABEL));
   node_details->SetString("payload.val",
       x509_certificate_model::HashCertSHA256(cert));
-  cert_sub_fields->Append(
-      base::WrapUnique(node_details = new base::DictionaryValue()));
+  cert_sub_fields->Append(base::MakeUnique<base::DictionaryValue>());
+  cert_sub_fields->GetDictionary(cert_sub_fields->GetSize() - 1, &node_details);
   node_details->SetString("label",
       l10n_util::GetStringUTF8(IDS_CERT_INFO_SHA1_FINGERPRINT_LABEL));
   node_details->SetString("payload.val",
       x509_certificate_model::HashCertSHA1(cert));
 
   // Send certificate information to javascript.
   web_ui()->CallJavascriptFunctionUnsafe("cert_viewer.getCertificateFields",
                                          root_list);
 }
 
 int CertificateViewerDialogHandler::GetCertificateIndex(
     const base::ListValue* args) const {
   int cert_index;
   double val;
   if (!(args->GetDouble(0, &val)))
     return -1;
   cert_index = static_cast<int>(val);
   if (cert_index < 0 || cert_index >= static_cast<int>(cert_chain_.size()))
     return -1;
   return cert_index;
 }