CodeHealth: Modernize base::Value code in components/domain_reliability/

Avoid DictionaryValue and ListValue, as they are deprecated. Get rid of
some temporary heap allocation.

Bug: 1187061, 1187062
Change-Id: I06631623c4c1d7288de93763b0c4ab45756f14d2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3009301
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#900505}

components/domain_reliability/beacon.cc

@@ -14,9 +14,6 @@
 
 namespace domain_reliability {
 
-using base::Value;
-using base::DictionaryValue;
-
 DomainReliabilityBeacon::DomainReliabilityBeacon() = default;
 
 DomainReliabilityBeacon::DomainReliabilityBeacon(
@@ -29,40 +26,41 @@ DomainReliabilityBeacon::~DomainReliabilityBeacon() {
   }
 }
 
-std::unique_ptr<Value> DomainReliabilityBeacon::ToValue(
+base::Value DomainReliabilityBeacon::ToValue(
     base::TimeTicks upload_time,
     base::TimeTicks last_network_change_time,
     const GURL& collector_url,
     const std::vector<std::unique_ptr<std::string>>& path_prefixes) const {
-  auto beacon_value = std::make_unique<DictionaryValue>();
+  base::Value beacon_value(base::Value::Type::DICTIONARY);
   DCHECK(url.is_valid());
   GURL sanitized_url = SanitizeURLForReport(url, collector_url, path_prefixes);
-  beacon_value->SetString("url", sanitized_url.spec());
-  beacon_value->SetString("status", status);
+  beacon_value.SetStringKey("url", sanitized_url.spec());
+  beacon_value.SetStringKey("status", status);
   if (!quic_error.empty())
-    beacon_value->SetString("quic_error", quic_error);
+    beacon_value.SetStringKey("quic_error", quic_error);
   if (chrome_error != net::OK) {
-    DictionaryValue failure_value;
-    failure_value.SetString("custom_error", net::ErrorToString(chrome_error));
-    beacon_value->SetKey("failure_data", std::move(failure_value));
+    base::Value failure_value(base::Value::Type::DICTIONARY);
+    failure_value.SetStringKey("custom_error",
+                               net::ErrorToString(chrome_error));
+    beacon_value.SetKey("failure_data", std::move(failure_value));
   }
-  beacon_value->SetString("server_ip", server_ip);
-  beacon_value->SetBoolean("was_proxied", was_proxied);
-  beacon_value->SetString("protocol", protocol);
+  beacon_value.SetStringKey("server_ip", server_ip);
+  beacon_value.SetBoolKey("was_proxied", was_proxied);
+  beacon_value.SetStringKey("protocol", protocol);
   if (details.quic_broken)
-    beacon_value->SetBoolean("quic_broken", details.quic_broken);
+    beacon_value.SetBoolKey("quic_broken", details.quic_broken);
   if (details.quic_port_migration_detected)
-    beacon_value->SetBoolean("quic_port_migration_detected",
-                             details.quic_port_migration_detected);
+    beacon_value.SetBoolKey("quic_port_migration_detected",
+                            details.quic_port_migration_detected);
   if (http_response_code >= 0)
-    beacon_value->SetInteger("http_response_code", http_response_code);
-  beacon_value->SetInteger("request_elapsed_ms", elapsed.InMilliseconds());
+    beacon_value.SetIntKey("http_response_code", http_response_code);
+  beacon_value.SetIntKey("request_elapsed_ms", elapsed.InMilliseconds());
   base::TimeDelta request_age = upload_time - start_time;
-  beacon_value->SetInteger("request_age_ms", request_age.InMilliseconds());
+  beacon_value.SetIntKey("request_age_ms", request_age.InMilliseconds());
   bool network_changed = last_network_change_time > start_time;
-  beacon_value->SetBoolean("network_changed", network_changed);
-  beacon_value->SetDouble("sample_rate", sample_rate);
-  return std::move(beacon_value);
+  beacon_value.SetBoolKey("network_changed", network_changed);
+  beacon_value.SetDoubleKey("sample_rate", sample_rate);
+  return beacon_value;
 }
 
 }  // namespace domain_reliability

components/domain_reliability/beacon.h

@@ -56,7 +56,7 @@ struct DOMAIN_RELIABILITY_EXPORT DomainReliabilityBeacon {
   // are being uploaded to a same-origin collector.
   // |path_prefixes| are used to include only a known-safe (not PII) prefix of
   // URLs when uploading to a non-same-origin collector.
-  std::unique_ptr<base::Value> ToValue(
+  base::Value ToValue(
       base::TimeTicks upload_time,
       base::TimeTicks last_network_change_time,
       const GURL& collector_url,

components/domain_reliability/context.cc

@@ -20,10 +20,6 @@
 #include "components/domain_reliability/util.h"
 #include "net/base/net_errors.h"
 
-using base::DictionaryValue;
-using base::ListValue;
-using base::Value;
-
 namespace domain_reliability {
 
 // static
@@ -89,17 +85,16 @@ void DomainReliabilityContext::ClearBeacons() {
   uploading_beacons_size_ = 0;
 }
 
-std::unique_ptr<Value> DomainReliabilityContext::GetWebUIData() const {
-  DictionaryValue* context_value = new DictionaryValue();
+base::Value DomainReliabilityContext::GetWebUIData() const {
+  base::Value context_value(base::Value::Type::DICTIONARY);
 
-  context_value->SetString("origin", config().origin.spec());
-  context_value->SetInteger("beacon_count", static_cast<int>(beacons_.size()));
-  context_value->SetInteger("uploading_beacon_count",
-      static_cast<int>(uploading_beacons_size_));
-  context_value->SetKey(
-      "scheduler", base::Value::FromUniquePtrValue(scheduler_.GetWebUIData()));
+  context_value.SetStringKey("origin", config().origin.spec());
+  context_value.SetIntKey("beacon_count", static_cast<int>(beacons_.size()));
+  context_value.SetIntKey("uploading_beacon_count",
+                          static_cast<int>(uploading_beacons_size_));
+  context_value.SetKey("scheduler", scheduler_.GetWebUIData());
 
-  return std::unique_ptr<Value>(context_value);
+  return context_value;
 }
 
 void DomainReliabilityContext::GetQueuedBeaconsForTesting(
@@ -162,10 +157,8 @@ void DomainReliabilityContext::StartUpload() {
   std::string report_json = "{}";
   int max_upload_depth = -1;
   bool wrote = base::JSONWriter::Write(
-      *CreateReport(upload_time_,
-                    collector_url,
-                    &max_upload_depth),
-                    &report_json);
+      CreateReport(upload_time_, collector_url, &max_upload_depth),
+      &report_json);
   DCHECK(wrote);
   DCHECK_NE(-1, max_upload_depth);
 
@@ -194,16 +187,15 @@ void DomainReliabilityContext::OnUploadComplete(
     scheduler_.OnBeaconAdded();
 }
 
-std::unique_ptr<const Value> DomainReliabilityContext::CreateReport(
-    base::TimeTicks upload_time,
-    const GURL& collector_url,
-    int* max_upload_depth_out) {
+base::Value DomainReliabilityContext::CreateReport(base::TimeTicks upload_time,
+                                                   const GURL& collector_url,
+                                                   int* max_upload_depth_out) {
   DCHECK_GT(beacons_.size(), 0u);
   DCHECK_EQ(0u, uploading_beacons_size_);
 
   int max_upload_depth = 0;
 
-  ListValue beacons_value;
+  base::Value beacons_value(base::Value::Type::LIST);
   for (const auto& beacon : beacons_) {
     // Only include beacons with a matching NetworkIsolationKey in the report.
     if (beacon->network_isolation_key !=
@@ -221,12 +213,12 @@ std::unique_ptr<const Value> DomainReliabilityContext::CreateReport(
 
   DCHECK_GT(uploading_beacons_size_, 0u);
 
-  std::unique_ptr<DictionaryValue> report_value(new DictionaryValue());
-  report_value->SetString("reporter", upload_reporter_string_);
-  report_value->SetKey("entries", std::move(beacons_value));
+  base::Value report_value(base::Value::Type::DICTIONARY);
+  report_value.SetStringKey("reporter", upload_reporter_string_);
+  report_value.SetKey("entries", std::move(beacons_value));
 
   *max_upload_depth_out = max_upload_depth;
-  return std::move(report_value);
+  return report_value;
 }
 
 void DomainReliabilityContext::CommitUpload() {

components/domain_reliability/context.h

@@ -67,7 +67,7 @@ class DOMAIN_RELIABILITY_EXPORT DomainReliabilityContext {
 
   // Gets a Value containing data that can be formatted into a web page for
   // debugging purposes.
-  std::unique_ptr<base::Value> GetWebUIData() const;
+  base::Value GetWebUIData() const;
 
   // Gets the beacons queued for upload in this context. `*beacons_out` will be
   // cleared and filled with pointers to the beacons; the pointers remain valid
@@ -91,9 +91,9 @@ class DOMAIN_RELIABILITY_EXPORT DomainReliabilityContext {
   // Creates a report from all beacons associated with
   // `uploading_beacons_network_isolation_key_`. Updates
   // `uploading_beacons_size_`.
-  std::unique_ptr<const base::Value> CreateReport(base::TimeTicks upload_time,
-                                                  const GURL& collector_url,
-                                                  int* max_beacon_depth_out);
+  base::Value CreateReport(base::TimeTicks upload_time,
+                           const GURL& collector_url,
+                           int* max_beacon_depth_out);
 
   // Uses the state remembered by `MarkUpload` to remove successfully uploaded
   // data but keep beacons and request counts added after the upload started.

components/domain_reliability/context_manager.cc

@@ -100,12 +100,11 @@ void DomainReliabilityContextManager::RemoveContexts(
   }
 }
 
-std::unique_ptr<base::Value> DomainReliabilityContextManager::GetWebUIData()
-    const {
-  std::unique_ptr<base::ListValue> contexts_value(new base::ListValue());
+base::Value DomainReliabilityContextManager::GetWebUIData() const {
+  base::Value contexts_value(base::Value::Type::LIST);
   for (const auto& context_entry : contexts_)
-    contexts_value->Append(context_entry.second->GetWebUIData());
-  return std::move(contexts_value);
+    contexts_value.Append(context_entry.second->GetWebUIData());
+  return contexts_value;
 }
 
 DomainReliabilityContext* DomainReliabilityContextManager::GetContext(

components/domain_reliability/context_manager.h

@@ -75,7 +75,7 @@ class DOMAIN_RELIABILITY_EXPORT DomainReliabilityContextManager {
   // |uploader_| needs to be set before any contexts are created.
   void SetUploader(DomainReliabilityUploader* uploader);
 
-  std::unique_ptr<base::Value> GetWebUIData() const;
+  base::Value GetWebUIData() const;
 
   size_t contexts_size_for_testing() const { return contexts_.size(); }
 

components/domain_reliability/context_unittest.cc

@@ -28,8 +28,6 @@
 namespace domain_reliability {
 namespace {
 
-using base::DictionaryValue;
-using base::ListValue;
 using base::Value;
 
 typedef std::vector<const DomainReliabilityBeacon*> BeaconVector;
@@ -85,36 +83,39 @@ std::string StatusFromInt(int i) {
   return base::StringPrintf("status%i.test", i);
 }
 
-template <typename ValueType,
-          bool (DictionaryValue::*GetValueType)(base::StringPiece, ValueType*)
-              const>
+template <typename ValueTypeFindResult,
+          typename ValueType,
+          ValueTypeFindResult (Value::*FindValueType)(base::StringPiece) const>
 struct HasValue {
-  bool operator()(const DictionaryValue& dict,
+  bool operator()(const Value& dict,
                   const std::string& key,
                   ValueType expected_value) {
-    ValueType actual_value;
-    bool got_value = (dict.*GetValueType)(key, &actual_value);
-    if (got_value)
-      EXPECT_EQ(expected_value, actual_value);
-    return got_value && (expected_value == actual_value);
+    ValueTypeFindResult actual_value = (dict.*FindValueType)(key);
+    if (actual_value)
+      EXPECT_EQ(expected_value, *actual_value);
+    return actual_value && (expected_value == *actual_value);
   }
 };
 
-HasValue<bool, &DictionaryValue::GetBoolean> HasBooleanValue;
-HasValue<double, &DictionaryValue::GetDouble> HasDoubleValue;
-HasValue<int, &DictionaryValue::GetInteger> HasIntegerValue;
-HasValue<std::string, &DictionaryValue::GetString> HasStringValue;
+HasValue<absl::optional<bool>, bool, &Value::FindBoolPath> HasBooleanValue;
+HasValue<absl::optional<double>, double, &Value::FindDoublePath> HasDoubleValue;
+HasValue<absl::optional<int>, int, &Value::FindIntPath> HasIntegerValue;
+HasValue<const std::string*, std::string, &Value::FindStringPath>
+    HasStringValue;
 
 bool GetEntryFromReport(const Value* report,
                         size_t index,
-                        const DictionaryValue** entry_out) {
-  const DictionaryValue* report_dict;
-  const ListValue* entries;
-
-  return report &&
-         report->GetAsDictionary(&report_dict) &&
-         report_dict->GetList("entries", &entries) &&
-         entries->GetDictionary(index, entry_out);
+                        const Value** entry_out) {
+  if (!report || !report->is_dict())
+    return false;
+  const Value* entries = report->FindListKey("entries");
+  if (!entries)
+    return false;
+  const Value& entry = entries->GetList()[index];
+  if (!entry.is_dict())
+    return false;
+  *entry_out = &entry;
+  return true;
 }
 
 class DomainReliabilityContextTest : public testing::Test {
@@ -363,7 +364,7 @@ TEST_F(DomainReliabilityContextTest, ReportUpload) {
 
   std::unique_ptr<Value> value =
       base::JSONReader::ReadDeprecated(upload_report());
-  const DictionaryValue* entry;
+  const Value* entry;
   ASSERT_TRUE(GetEntryFromReport(value.get(), 0, &entry));
   EXPECT_TRUE(HasStringValue(*entry, "failure_data.custom_error",
                              "net::ERR_CONNECTION_RESET"));
@@ -688,7 +689,7 @@ TEST_F(DomainReliabilityContextTest, NetworkChanged) {
 
   std::unique_ptr<Value> value =
       base::JSONReader::ReadDeprecated(upload_report());
-  const DictionaryValue* entry;
+  const Value* entry;
   ASSERT_TRUE(GetEntryFromReport(value.get(), 0, &entry));
   EXPECT_TRUE(HasBooleanValue(*entry, "network_changed", true));
 
@@ -719,7 +720,7 @@ TEST_F(DomainReliabilityContextTest,
 
   std::unique_ptr<Value> value =
       base::JSONReader::ReadDeprecated(upload_report());
-  const DictionaryValue* entry;
+  const Value* entry;
   ASSERT_TRUE(GetEntryFromReport(value.get(), 0, &entry));
 
   EXPECT_TRUE(HasBooleanValue(*entry, "quic_broken", true));
@@ -753,7 +754,7 @@ TEST_F(DomainReliabilityContextTest,
 
   std::unique_ptr<Value> value =
       base::JSONReader::ReadDeprecated(upload_report());
-  const DictionaryValue* entry;
+  const Value* entry;
   ASSERT_TRUE(GetEntryFromReport(value.get(), 0, &entry));
 
   EXPECT_TRUE(HasStringValue(*entry, "status", "tcp.connection_reset"));
@@ -788,7 +789,7 @@ TEST_F(DomainReliabilityContextTest,
 
   std::unique_ptr<Value> value =
       base::JSONReader::ReadDeprecated(upload_report());
-  const DictionaryValue* entry;
+  const Value* entry;
   ASSERT_TRUE(GetEntryFromReport(value.get(), 0, &entry));
   EXPECT_TRUE(HasBooleanValue(*entry, "quic_broken", true));
   EXPECT_TRUE(HasStringValue(*entry, "status", "tcp.connection_reset"));
@@ -834,7 +835,7 @@ TEST_F(DomainReliabilityContextTest, FractionalSampleRate) {
 
   std::unique_ptr<Value> value =
       base::JSONReader::ReadDeprecated(upload_report());
-  const DictionaryValue* entry;
+  const Value* entry;
   ASSERT_TRUE(GetEntryFromReport(value.get(), 0, &entry));
   EXPECT_TRUE(HasDoubleValue(*entry, "sample_rate", 0.5));
 

components/domain_reliability/monitor.cc

@@ -156,12 +156,10 @@ void DomainReliabilityMonitor::ClearBrowsingData(
   }
 }
 
-std::unique_ptr<base::Value> DomainReliabilityMonitor::GetWebUIData() const {
-  std::unique_ptr<base::DictionaryValue> data_value(
-      new base::DictionaryValue());
-  data_value->SetKey("contexts", base::Value::FromUniquePtrValue(
-                                     context_manager_.GetWebUIData()));
-  return std::move(data_value);
+base::Value DomainReliabilityMonitor::GetWebUIData() const {
+  base::Value data_value(base::Value::Type::DICTIONARY);
+  data_value.SetKey("contexts", context_manager_.GetWebUIData());
+  return data_value;
 }
 
 const DomainReliabilityContext* DomainReliabilityMonitor::AddContextForTesting(

components/domain_reliability/monitor.h

@@ -125,7 +125,7 @@ class DOMAIN_RELIABILITY_EXPORT DomainReliabilityMonitor
 
   // Gets a Value containing data that can be formatted into a web page for
   // debugging purposes.
-  std::unique_ptr<base::Value> GetWebUIData() const;
+  base::Value GetWebUIData() const;
 
   // Returns pointer to the added context.
   const DomainReliabilityContext* AddContextForTesting(

components/domain_reliability/scheduler.cc

@@ -159,41 +159,41 @@ void DomainReliabilityScheduler::OnUploadComplete(
   MaybeScheduleUpload();
 }
 
-std::unique_ptr<base::Value> DomainReliabilityScheduler::GetWebUIData() const {
+base::Value DomainReliabilityScheduler::GetWebUIData() const {
   base::TimeTicks now = time_->NowTicks();
 
-  std::unique_ptr<base::DictionaryValue> data(new base::DictionaryValue());
+  base::Value data(base::Value::Type::DICTIONARY);
 
-  data->SetBoolean("upload_pending", upload_pending_);
-  data->SetBoolean("upload_scheduled", upload_scheduled_);
-  data->SetBoolean("upload_running", upload_running_);
+  data.SetBoolKey("upload_pending", upload_pending_);
+  data.SetBoolKey("upload_scheduled", upload_scheduled_);
+  data.SetBoolKey("upload_running", upload_running_);
 
-  data->SetInteger("scheduled_min", (scheduled_min_time_ - now).InSeconds());
-  data->SetInteger("scheduled_max", (scheduled_max_time_ - now).InSeconds());
+  data.SetIntKey("scheduled_min", (scheduled_min_time_ - now).InSeconds());
+  data.SetIntKey("scheduled_max", (scheduled_max_time_ - now).InSeconds());
 
-  data->SetInteger("collector_index", static_cast<int>(collector_index_));
+  data.SetIntKey("collector_index", static_cast<int>(collector_index_));
 
   if (last_upload_finished_) {
-    base::DictionaryValue last;
-    last.SetInteger("start_time", (now - last_upload_start_time_).InSeconds());
-    last.SetInteger("end_time", (now - last_upload_end_time_).InSeconds());
-    last.SetInteger("collector_index",
-                    static_cast<int>(last_upload_collector_index_));
-    last.SetBoolean("success", last_upload_success_);
-    data->SetKey("last_upload", std::move(last));
+    base::Value last(base::Value::Type::DICTIONARY);
+    last.SetIntKey("start_time", (now - last_upload_start_time_).InSeconds());
+    last.SetIntKey("end_time", (now - last_upload_end_time_).InSeconds());
+    last.SetIntKey("collector_index",
+                   static_cast<int>(last_upload_collector_index_));
+    last.SetBoolKey("success", last_upload_success_);
+    data.SetKey("last_upload", std::move(last));
   }
 
-  base::ListValue collectors_value;
+  base::Value collectors_value(base::Value::Type::LIST);
   for (const auto& collector : collectors_) {
-    std::unique_ptr<base::DictionaryValue> value(new base::DictionaryValue());
-    value->SetInteger("failures", collector->failure_count());
-    value->SetInteger("next_upload",
-        (collector->GetReleaseTime() - now).InSeconds());
+    base::Value value(base::Value::Type::DICTIONARY);
+    value.SetIntKey("failures", collector->failure_count());
+    value.SetIntKey("next_upload",
+                    (collector->GetReleaseTime() - now).InSeconds());
     collectors_value.Append(std::move(value));
   }
-  data->SetKey("collectors", std::move(collectors_value));
+  data.SetKey("collectors", std::move(collectors_value));
 
-  return std::move(data);
+  return data;
 }
 
 void DomainReliabilityScheduler::MakeDeterministicForTesting() {

components/domain_reliability/scheduler.h

@@ -73,7 +73,7 @@ class DOMAIN_RELIABILITY_EXPORT DomainReliabilityScheduler {
   // passed to the upload callback by the Uploader.
   void OnUploadComplete(const DomainReliabilityUploader::UploadResult& result);
 
-  std::unique_ptr<base::Value> GetWebUIData() const;
+  base::Value GetWebUIData() const;
 
   // Disables jitter in BackoffEntries to make scheduling deterministic for
   // unit tests.

services/network/network_context.cc

@@ -1056,8 +1056,7 @@ void NetworkContext::GetDomainReliabilityJSON(
     return;
   }
 
-  std::move(callback).Run(
-      std::move(*domain_reliability_monitor_->GetWebUIData()));
+  std::move(callback).Run(domain_reliability_monitor_->GetWebUIData());
 }
 
 void NetworkContext::CloseAllConnections(CloseAllConnectionsCallback callback) {