Complete removal of DictionaryValue from net/log

I also took this opportunity to silence a clang-tidy warning in values.h. Given that there is already a long comment about why the linter is wrong in this case, it seems only natural to let the linter know about it, too.

Bug: 646113
Change-Id: I59ae89f42731445789638c4c25840249263127fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2285305
Commit-Queue: Panos Astithas <pastithas@google.com>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Eric Roman <eroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#786620}

base/values.h

@@ -121,7 +121,7 @@ class BASE_EXPORT Value {
   static const ListValue& AsListValue(const Value& val);
 
   Value(Value&& that) noexcept;
-  Value() noexcept {}  // A null value
+  Value() noexcept {}  // A null value. NOLINT(modernize-use-equals-default)
   // Fun fact: using '= default' above instead of '{}' does not work because
   // the compiler complains that the default constructor was deleted since
   // the inner union contains fields with non-default constructors.

net/log/file_net_log_observer.cc

@@ -481,8 +481,7 @@ FileNetLogObserver::FileNetLogObserver(
       write_queue_(std::move(write_queue)),
       file_writer_(std::move(file_writer)) {
   if (!constants)
-    constants = base::DictionaryValue::From(
-        base::Value::ToUniquePtrValue(GetNetConstants()));
+    constants = base::Value::ToUniquePtrValue(GetNetConstants());
   file_task_runner_->PostTask(
       FROM_HERE, base::BindOnce(&FileNetLogObserver::FileWriter::Initialize,
                                 base::Unretained(file_writer_.get()),

net/log/file_net_log_observer_unittest.cc

@@ -98,23 +98,20 @@ void AddEntries(FileNetLogObserver* logger,
 // and polled data).
 struct ParsedNetLog {
   ::testing::AssertionResult InitFromFileContents(const std::string& input);
-  const base::DictionaryValue* GetEvent(size_t i) const;
+  const base::Value* GetEvent(size_t i) const;
 
   // Initializes the ParsedNetLog by parsing a JSON file.
-  // Owner for the Value tree.
-  base::Value container;
-
-  // A dictionary for the entire netlog.
-  const base::DictionaryValue* root = nullptr;
+  // Owner for the Value tree and a dictionary for the entire netlog.
+  base::Value root;
 
   // The constants dictionary.
-  const base::DictionaryValue* constants = nullptr;
+  const base::Value* constants = nullptr;
 
   // The events list.
-  const base::ListValue* events = nullptr;
+  const base::Value* events = nullptr;
 
   // The optional polled data (may be nullptr).
-  const base::DictionaryValue* polled_data = nullptr;
+  const base::Value* polled_data = nullptr;
 };
 
 ::testing::AssertionResult ParsedNetLog::InitFromFileContents(
@@ -128,40 +125,38 @@ struct ParsedNetLog {
   if (!parsed_json.value) {
     return ::testing::AssertionFailure() << parsed_json.error_message;
   }
-  container = std::move(*parsed_json.value);
+  root = std::move(*parsed_json.value);
 
-  if (!container.GetAsDictionary(&root)) {
+  if (!root.is_dict()) {
     return ::testing::AssertionFailure() << "Not a dictionary";
   }
 
-  if (!root->GetList("events", &events)) {
+  events = root.FindPath("events");
+  if (!events || !events->is_list()) {
     return ::testing::AssertionFailure() << "No events list";
   }
 
-  if (!root->GetDictionary("constants", &constants)) {
+  constants = root.FindDictPath("constants");
+  if (!constants) {
     return ::testing::AssertionFailure() << "No constants dictionary";
   }
 
   // Polled data is optional (ignore success).
-  root->GetDictionary("polledData", &polled_data);
+  polled_data = root.FindDictPath("polledData");
 
   return ::testing::AssertionSuccess();
 }
 
 // Returns the event at index |i|, or nullptr if there is none.
-const base::DictionaryValue* ParsedNetLog::GetEvent(size_t i) const {
-  if (!events || i >= events->GetSize())
-    return nullptr;
-
-  const base::Value* value;
-  if (!events->Get(i, &value))
+const base::Value* ParsedNetLog::GetEvent(size_t i) const {
+  if (!events || i >= events->GetList().size())
     return nullptr;
 
-  const base::DictionaryValue* dict;
-  if (!value->GetAsDictionary(&dict))
+  const base::Value& value = events->GetList()[i];
+  if (!value.is_dict())
     return nullptr;
 
-  return dict;
+  return &value;
 }
 
 // Creates a ParsedNetLog by reading a NetLog from a file. Returns nullptr on
@@ -193,33 +188,32 @@ void VerifyEventsInLog(const ParsedNetLog* log,
                        size_t num_events_saved) {
   ASSERT_TRUE(log);
   ASSERT_LE(num_events_saved, num_events_emitted);
-  ASSERT_EQ(num_events_saved, log->events->GetSize());
+  ASSERT_EQ(num_events_saved, log->events->GetList().size());
 
   // The last |num_events_saved| should all be sequential, with the last one
   // being numbered |num_events_emitted - 1|.
   for (size_t i = 0; i < num_events_saved; ++i) {
-    const base::DictionaryValue* event = log->GetEvent(i);
+    const base::Value* event = log->GetEvent(i);
     ASSERT_TRUE(event);
 
     size_t expected_source_id = num_events_emitted - num_events_saved + i;
 
-    const base::Value* id_value = nullptr;
-    ASSERT_TRUE(event->Get("source.id", &id_value));
-    int actual_id;
-    ASSERT_TRUE(id_value->GetAsInteger(&actual_id));
+    const base::Value* id_value = event->FindPath("source.id");
+    ASSERT_TRUE(id_value);
+    int actual_id = id_value->GetInt();
     ASSERT_EQ(static_cast<int>(expected_source_id), actual_id);
   }
 }
 
 // Helper that checks whether |dict| has a string property at |key| having
 // |value|.
-void ExpectDictionaryContainsProperty(const base::DictionaryValue* dict,
+void ExpectDictionaryContainsProperty(const base::Value* dict,
                                       const std::string& key,
                                       const std::string& value) {
   ASSERT_TRUE(dict);
-  std::string actual_value;
-  ASSERT_TRUE(dict->GetString(key, &actual_value));
-  ASSERT_EQ(value, actual_value);
+  ASSERT_TRUE(dict->is_dict());
+  const std::string* actual_value = dict->FindStringPath(key);
+  ASSERT_EQ(value, *actual_value);
 }
 
 // Used for tests that are common to both bounded and unbounded modes of the
@@ -445,7 +439,7 @@ TEST_P(FileNetLogObserverTest, GeneratesValidJSONWithNoEvents) {
   // Verify the written log.
   std::unique_ptr<ParsedNetLog> log = ReadNetLogFromDisk(log_path_);
   ASSERT_TRUE(log);
-  ASSERT_EQ(0u, log->events->GetSize());
+  ASSERT_EQ(0u, log->events->GetList().size());
 }
 
 TEST_P(FileNetLogObserverTest, GeneratesValidJSONWithOneEvent) {
@@ -463,7 +457,7 @@ TEST_P(FileNetLogObserverTest, GeneratesValidJSONWithOneEvent) {
   // Verify the written log.
   std::unique_ptr<ParsedNetLog> log = ReadNetLogFromDisk(log_path_);
   ASSERT_TRUE(log);
-  ASSERT_EQ(1u, log->events->GetSize());
+  ASSERT_EQ(1u, log->events->GetList().size());
 }
 
 TEST_P(FileNetLogObserverTest, GeneratesValidJSONWithOneEventPreExisting) {
@@ -481,7 +475,7 @@ TEST_P(FileNetLogObserverTest, GeneratesValidJSONWithOneEventPreExisting) {
   // Verify the written log.
   std::unique_ptr<ParsedNetLog> log = ReadNetLogFromDisk(log_path_);
   ASSERT_TRUE(log);
-  ASSERT_EQ(1u, log->events->GetSize());
+  ASSERT_EQ(1u, log->events->GetList().size());
 }
 
 TEST_P(FileNetLogObserverTest, PreExistingFileBroken) {
@@ -510,8 +504,9 @@ TEST_P(FileNetLogObserverTest, CustomConstants) {
 
   const char kConstantKey[] = "magic";
   const char kConstantString[] = "poney";
-  std::unique_ptr<base::DictionaryValue> constants(new base::DictionaryValue());
-  constants->SetString(kConstantKey, kConstantString);
+  std::unique_ptr<base::Value> constants(
+      new base::Value(base::Value::Type::DICTIONARY));
+  constants->SetStringPath(kConstantKey, kConstantString);
 
   CreateAndStartObserving(std::move(constants));
 
@@ -536,9 +531,10 @@ TEST_P(FileNetLogObserverTest, GeneratesValidJSONWithPolledData) {
   // Create dummy polled data
   const char kDummyPolledDataPath[] = "dummy_path";
   const char kDummyPolledDataString[] = "dummy_info";
-  std::unique_ptr<base::DictionaryValue> dummy_polled_data =
-      std::make_unique<base::DictionaryValue>();
-  dummy_polled_data->SetString(kDummyPolledDataPath, kDummyPolledDataString);
+  std::unique_ptr<base::Value> dummy_polled_data(
+      new base::Value(base::Value::Type::DICTIONARY));
+  dummy_polled_data->SetStringPath(kDummyPolledDataPath,
+                                   kDummyPolledDataString);
 
   logger_->StopObserving(std::move(dummy_polled_data), closure.closure());
 
@@ -547,7 +543,7 @@ TEST_P(FileNetLogObserverTest, GeneratesValidJSONWithPolledData) {
   // Verify the written log.
   std::unique_ptr<ParsedNetLog> log = ReadNetLogFromDisk(log_path_);
   ASSERT_TRUE(log);
-  ASSERT_EQ(0u, log->events->GetSize());
+  ASSERT_EQ(0u, log->events->GetList().size());
 
   // Make sure additional information is present and validate it.
   ASSERT_TRUE(log->polled_data);
@@ -619,7 +615,8 @@ TEST_P(FileNetLogObserverTest, AddEventsFromMultipleThreads) {
   std::unique_ptr<ParsedNetLog> log = ReadNetLogFromDisk(log_path_);
   ASSERT_TRUE(log);
   // Check that the expected number of events were written to disk.
-  EXPECT_EQ(kNumEventsAddedPerThread * kNumThreads, log->events->GetSize());
+  EXPECT_EQ(kNumEventsAddedPerThread * kNumThreads,
+            log->events->GetList().size());
 
 #if defined(OS_FUCHSIA)
   LOG(ERROR) << "Teardown.";
@@ -933,7 +930,7 @@ TEST_F(FileNetLogObserverBoundedTest, BlockEventsFile0) {
   // Verify the written log.
   std::unique_ptr<ParsedNetLog> log = ReadNetLogFromDisk(log_path_);
   ASSERT_TRUE(log);
-  ASSERT_EQ(0u, log->events->GetSize());
+  ASSERT_EQ(0u, log->events->GetList().size());
 }
 
 // Make sure that when using bounded mode with a pre-existing output file,
@@ -992,7 +989,7 @@ TEST_F(FileNetLogObserverBoundedTest, LargeWriteQueueSize) {
   // Verify the written log.
   std::unique_ptr<ParsedNetLog> log = ReadNetLogFromDisk(log_path_);
   ASSERT_TRUE(log);
-  ASSERT_EQ(3u, log->events->GetSize());
+  ASSERT_EQ(3u, log->events->GetList().size());
 }
 
 void AddEntriesViaNetLog(NetLog* net_log, int num_entries) {

net/log/net_log_source.cc

@@ -60,17 +60,31 @@ base::Value NetLogSource::ToEventParameters() const {
 // static
 bool NetLogSource::FromEventParameters(const base::Value* event_params,
                                        NetLogSource* source) {
-  const base::DictionaryValue* dict = nullptr;
-  const base::DictionaryValue* source_dict = nullptr;
+  const base::Value* source_dict = nullptr;
   int source_id = -1;
   int source_type = static_cast<int>(NetLogSourceType::COUNT);
-  if (!event_params || !event_params->GetAsDictionary(&dict) ||
-      !dict->GetDictionary("source_dependency", &source_dict) ||
-      !source_dict->GetInteger("id", &source_id) ||
-      !source_dict->GetInteger("type", &source_type)) {
+  if (!event_params) {
     *source = NetLogSource();
     return false;
   }
+  source_dict = event_params->FindDictKey("source_dependency");
+  if (!source_dict) {
+    *source = NetLogSource();
+    return false;
+  }
+  base::Optional<int> opt_int;
+  opt_int = source_dict->FindIntKey("id");
+  if (!opt_int) {
+    *source = NetLogSource();
+    return false;
+  }
+  source_id = opt_int.value();
+  opt_int = source_dict->FindIntKey("type");
+  if (!opt_int) {
+    *source = NetLogSource();
+    return false;
+  }
+  source_type = opt_int.value();
 
   DCHECK_GE(source_id, 0);
   DCHECK_LT(source_type, static_cast<int>(NetLogSourceType::COUNT));

net/log/net_log_unittest.cc

@@ -31,9 +31,9 @@ base::Value CaptureModeToValue(NetLogCaptureMode capture_mode) {
 }
 
 base::Value NetCaptureModeParams(NetLogCaptureMode capture_mode) {
-  base::DictionaryValue dict;
+  base::Value dict(base::Value::Type::DICTIONARY);
   dict.SetKey("capture_mode", CaptureModeToValue(capture_mode));
-  return std::move(dict);
+  return dict;
 }
 
 TEST(NetLogTest, BasicGlobalEvents) {
@@ -212,19 +212,17 @@ class LoggingObserver : public NetLog::ThreadSafeObserver {
   }
 
   void OnAddEntry(const NetLogEntry& entry) override {
-    std::unique_ptr<base::DictionaryValue> dict = base::DictionaryValue::From(
-        base::Value::ToUniquePtrValue(entry.ToValue()));
+    std::unique_ptr<base::Value> dict =
+        base::Value::ToUniquePtrValue(entry.ToValue());
     ASSERT_TRUE(dict);
     values_.push_back(std::move(dict));
   }
 
   size_t GetNumValues() const { return values_.size(); }
-  base::DictionaryValue* GetValue(size_t index) const {
-    return values_[index].get();
-  }
+  base::Value* GetValue(size_t index) const { return values_[index].get(); }
 
  private:
-  std::vector<std::unique_ptr<base::DictionaryValue>> values_;
+  std::vector<std::unique_ptr<base::Value>> values_;
 };
 
 void AddEvent(NetLog* net_log) {
@@ -416,14 +414,16 @@ TEST(NetLogTest, NetLogTwoObservers) {
 
   // Add event and make sure both observers receive it at their respective log
   // levels.
-  int param;
+  base::Optional<int> param;
   AddEvent(&net_log);
   ASSERT_EQ(1U, observer[0].GetNumValues());
-  ASSERT_TRUE(observer[0].GetValue(0)->GetInteger("params", &param));
-  EXPECT_EQ(CaptureModeToInt(observer[0].capture_mode()), param);
+  param = observer[0].GetValue(0)->FindIntKey("params");
+  ASSERT_TRUE(param);
+  EXPECT_EQ(CaptureModeToInt(observer[0].capture_mode()), param.value());
   ASSERT_EQ(1U, observer[1].GetNumValues());
-  ASSERT_TRUE(observer[1].GetValue(0)->GetInteger("params", &param));
-  EXPECT_EQ(CaptureModeToInt(observer[1].capture_mode()), param);
+  param = observer[1].GetValue(0)->FindIntKey("params");
+  ASSERT_TRUE(param);
+  EXPECT_EQ(CaptureModeToInt(observer[1].capture_mode()), param.value());
 
   // Remove second observer.
   net_log.RemoveObserver(&observer[1]);

net/log/trace_net_log_observer_unittest.cc

@@ -45,13 +45,33 @@ struct TraceEntryInfo {
   std::string source_type;
 };
 
-TraceEntryInfo GetTraceEntryInfoFromValue(const base::DictionaryValue& value) {
+TraceEntryInfo GetTraceEntryInfoFromValue(const base::Value& value) {
   TraceEntryInfo info;
-  EXPECT_TRUE(value.GetString("cat", &info.category));
-  EXPECT_TRUE(value.GetString("id", &info.id));
-  EXPECT_TRUE(value.GetString("ph", &info.phase));
-  EXPECT_TRUE(value.GetString("name", &info.name));
-  EXPECT_TRUE(value.GetString("args.source_type", &info.source_type));
+  if (const std::string* cat = value.FindStringKey("cat")) {
+    info.category = *cat;
+  } else {
+    ADD_FAILURE() << "Missing 'cat'";
+  }
+  if (const std::string* id = value.FindStringKey("id")) {
+    info.id = *id;
+  } else {
+    ADD_FAILURE() << "Missing 'id'";
+  }
+  if (const std::string* ph = value.FindStringKey("ph")) {
+    info.phase = *ph;
+  } else {
+    ADD_FAILURE() << "Missing 'ph'";
+  }
+  if (const std::string* name = value.FindStringKey("name")) {
+    info.name = *name;
+  } else {
+    ADD_FAILURE() << "Missing 'name'";
+  }
+  if (const std::string* type = value.FindStringPath("args.source_type")) {
+    info.source_type = *type;
+  } else {
+    ADD_FAILURE() << "Missing 'args.source_type'";
+  }
 
   return info;
 }
@@ -137,18 +157,18 @@ class TraceNetLogObserverTest : public TestWithTaskEnvironment {
     std::unique_ptr<base::ListValue> filtered_trace_events(
         new base::ListValue());
     for (size_t i = 0; i < trace_events.GetSize(); i++) {
-      const base::DictionaryValue* dict = nullptr;
-      if (!trace_events.GetDictionary(i, &dict)) {
+      const base::Value* dict = &trace_events.GetList()[i];
+      if (!dict->is_dict()) {
         ADD_FAILURE() << "Unexpected non-dictionary event in trace_events";
         continue;
       }
-      std::string category;
-      if (!dict->GetString("cat", &category)) {
+      const std::string* category = dict->FindStringPath("cat");
+      if (!category) {
         ADD_FAILURE()
             << "Unexpected item without a category field in trace_events";
         continue;
       }
-      if (category != kNetLogTracingCategory)
+      if (*category != kNetLogTracingCategory)
         continue;
       filtered_trace_events->Append(dict->CreateDeepCopy());
     }
@@ -219,12 +239,14 @@ TEST_F(TraceNetLogObserverTest, TraceEventCaptured) {
   EndTraceAndFlush();
   trace_net_log_observer()->StopWatchForTraceStart();
   EXPECT_EQ(3u, trace_events()->GetSize());
-  const base::DictionaryValue* item1 = nullptr;
-  ASSERT_TRUE(trace_events()->GetDictionary(0, &item1));
-  const base::DictionaryValue* item2 = nullptr;
-  ASSERT_TRUE(trace_events()->GetDictionary(1, &item2));
-  const base::DictionaryValue* item3 = nullptr;
-  ASSERT_TRUE(trace_events()->GetDictionary(2, &item3));
+  const base::Value* item1 = &trace_events()->GetList()[0];
+  ASSERT_TRUE(item1->is_dict());
+  const base::Value* item2 = &trace_events()->GetList()[1];
+  ;
+  ASSERT_TRUE(item2->is_dict());
+  const base::Value* item3 = &trace_events()->GetList()[2];
+  ;
+  ASSERT_TRUE(item3->is_dict());
 
   TraceEntryInfo actual_item1 = GetTraceEntryInfoFromValue(*item1);
   TraceEntryInfo actual_item2 = GetTraceEntryInfoFromValue(*item2);
@@ -272,10 +294,10 @@ TEST_F(TraceNetLogObserverTest, EnableAndDisableTracing) {
   auto entries = net_log()->GetEntries();
   EXPECT_EQ(3u, entries.size());
   EXPECT_EQ(2u, trace_events()->GetSize());
-  const base::DictionaryValue* item1 = nullptr;
-  ASSERT_TRUE(trace_events()->GetDictionary(0, &item1));
-  const base::DictionaryValue* item2 = nullptr;
-  ASSERT_TRUE(trace_events()->GetDictionary(1, &item2));
+  const base::Value* item1 = &trace_events()->GetList()[0];
+  ASSERT_TRUE(item1->is_dict());
+  const base::Value* item2 = &trace_events()->GetList()[1];
+  ASSERT_TRUE(item2->is_dict());
 
   TraceEntryInfo actual_item1 = GetTraceEntryInfoFromValue(*item1);
   TraceEntryInfo actual_item2 = GetTraceEntryInfoFromValue(*item2);
@@ -312,8 +334,8 @@ TEST_F(TraceNetLogObserverTest, DestroyObserverWhileTracing) {
   EXPECT_EQ(2u, entries.size());
   EXPECT_EQ(1u, trace_events()->GetSize());
 
-  const base::DictionaryValue* item1 = nullptr;
-  ASSERT_TRUE(trace_events()->GetDictionary(0, &item1));
+  const base::Value* item1 = &trace_events()->GetList()[0];
+  ASSERT_TRUE(item1->is_dict());
 
   TraceEntryInfo actual_item1 = GetTraceEntryInfoFromValue(*item1);
   EXPECT_EQ(kNetLogTracingCategory, actual_item1.category);
@@ -392,10 +414,10 @@ TEST_F(TraceNetLogObserverTest, EventsWithAndWithoutParameters) {
   auto entries = net_log()->GetEntries();
   EXPECT_EQ(2u, entries.size());
   EXPECT_EQ(2u, trace_events()->GetSize());
-  const base::DictionaryValue* item1 = nullptr;
-  ASSERT_TRUE(trace_events()->GetDictionary(0, &item1));
-  const base::DictionaryValue* item2 = nullptr;
-  ASSERT_TRUE(trace_events()->GetDictionary(1, &item2));
+  const base::Value* item1 = &trace_events()->GetList()[0];
+  ASSERT_TRUE(item1->is_dict());
+  const base::Value* item2 = &trace_events()->GetList()[1];
+  ASSERT_TRUE(item2->is_dict());
 
   TraceEntryInfo actual_item1 = GetTraceEntryInfoFromValue(*item1);
   TraceEntryInfo actual_item2 = GetTraceEntryInfoFromValue(*item2);
@@ -417,13 +439,13 @@ TEST_F(TraceNetLogObserverTest, EventsWithAndWithoutParameters) {
   EXPECT_EQ(NetLog::SourceTypeToString(entries[1].source.type),
             actual_item2.source_type);
 
-  std::string item1_params;
-  const base::DictionaryValue* item2_params;
-  EXPECT_TRUE(item1->GetString("args.params.foo", &item1_params));
-  EXPECT_EQ("bar", item1_params);
+  const std::string* item1_params = item1->FindStringPath("args.params.foo");
+  ASSERT_TRUE(item1_params);
+  EXPECT_EQ("bar", *item1_params);
 
-  EXPECT_TRUE(item2->GetDictionary("args.params", &item2_params));
-  EXPECT_TRUE(item2_params->empty());
+  const base::Value* item2_params = item2->FindDictPath("args.params");
+  ASSERT_TRUE(item2_params);
+  EXPECT_TRUE(item2_params->DictEmpty());
 }
 
 TEST(TraceNetLogObserverCategoryTest, DisabledCategory) {