Remove raw base::DictionaryValue::SetWithoutPathExpansion in //chrome

chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc

Index: chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc
diff --git a/chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc b/chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc
index 8da6f1930fb8eb3b08cadf222148041b1f68de08..813b5374519f4036721608a76512629229355dbe 100644
--- a/chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc
+++ b/chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc
@@ -16,6 +16,7 @@
 
 #include "chrome/browser/safe_browsing/incident_reporting/platform_state_store.h"
 
+#include "base/memory/ptr_util.h"
 #include "base/values.h"
 
 #if defined(USE_PLATFORM_STATE_STORE)
@@ -115,8 +116,9 @@ void RestoreFromProtobuf(
     base::DictionaryValue* type_dict = nullptr;
     if (!value_dict->GetDictionaryWithoutPathExpansion(type_string,
                                                        &type_dict)) {
-      type_dict = new base::DictionaryValue();
-      value_dict->SetWithoutPathExpansion(type_string, type_dict);
+      value_dict->SetWithoutPathExpansion(
+          type_string, base::MakeUnique<base::DictionaryValue>());
+      value_dict->GetDictionaryWithoutPathExpansion(type_string, &type_dict);

[jdoerrie, 2017/04/27 15:50:29]

While the call to GetWithoutPathExpansion is not strictly necessary yet, it
prepares the code for the upcoming change to base::Value::DictStorage after
which calls to Set* will invalidate pointers to the passed in Value. Querying
the pointer immediately after Set will be necessary.

This also applies to similar changes below.

[Nico, 2017/04/27 15:58:25]

That seems kind of inefficient, maybe SetWithoutPathExpansion() should return
the inserted DictionaryValue() so one doesn't have to re-query?

[jdoerrie, 2017/04/27 16:09:31]

I made a similar suggestion when doing the ListValue change which involved
Append returning a pointer to the just added element. brettw@ argued against it,
basically saying we should try to mimic push_back as much as possible:
https://codereview.chromium.org/2740143002/diff/140001/base/values.h#newcode445

Here the situation is different though, since all of
std::map::{insert,emplace,operator[]} either return an iterator or a reference
to the inserted element.

What do you think, brettw@? Do you agree with DictionaryValue::Set* returning
pointers to the inserted Values?

[brettw, 2017/04/27 21:54:11]

I'm happy to treat this like std::map::insert and return some kind of pointer or
iterator thingy. For lists it's easy to get access to the inserted element if
you need it, but that's not the case here.

     }
     RestoreOfTypeFromProtobuf(type_incidents.incidents().key_to_digest(),
                               type_dict);