Make SDCH dictionaries persistent across browser restart.

chrome/browser/profiles/profile_impl_io_data.cc

Index: chrome/browser/profiles/profile_impl_io_data.cc
diff --git a/chrome/browser/profiles/profile_impl_io_data.cc b/chrome/browser/profiles/profile_impl_io_data.cc
index 5af118651a2aaa4c21d25a03237586d558a741d4..7dc0bd5b6728dd804f97a1948e573cf056bb9628 100644
--- a/chrome/browser/profiles/profile_impl_io_data.cc
+++ b/chrome/browser/profiles/profile_impl_io_data.cc
@@ -4,11 +4,15 @@
 
 #include "chrome/browser/profiles/profile_impl_io_data.h"
 
+#include <set>
+
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/field_trial.h"
+#include "base/prefs/json_pref_store.h"
+#include "base/prefs/pref_filter.h"
 #include "base/prefs/pref_member.h"
 #include "base/prefs/pref_service.h"
 #include "base/profiler/scoped_tracker.h"
@@ -424,6 +428,21 @@ void ProfileImplIOData::InitializeInternal(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "436671 ProfileImplIOData::InitializeInternal"));
 
+  // Confirm that there aren't duplicate preference names used by different
+  // services within the network stack.
+  std::set<std::string> preference_names;
+  DCHECK(preference_names.insert(net::SdchOwner::PreferenceName()).second);

[mmenke, 2015/02/04 21:14:39]

DCHECKs with side effects just seem like a bad idea to me.  And I'm extremely
skeptical about expecting people will remember to update this if they add a new
preference to this store.

Also, there's already PrefRegistry...If we want to do this, should we be using
that instead?  The way they work is the object that cares about the preference
does the registering, I believe.

[Randy Smith (Not in Mondays), 2015/02/04 21:20:24]

Well, given that if the DCHECK is compiled out, there won't be any point to
doing this, a DCHECK with a side effect seemed like it pretty precisely
encapsulated my intent.  I could make it explicit with an #ifndef NDEBUG if you
want, but I'm not sure I see the point.
Why is this any worse than pref registry?  The registration function still needs
to be called from some central place (e.g. IOThread::IOThread, search for
SetDefaultPrefValue).  
So I considered PrefRegistry but rejected it for reasons I sketched out in the
main comment on my last post; the quick summary is that PrefRegistry also offers
default value setting, which I don't see a way to integrate cleanly with
JsonPrefStore without changes to JsonPrefStore, which seemed like too much
mission creeep.

[mmenke, 2015/02/04 21:34:32]

Only one reason.  Suppose people see what you're doing, and want to copy it... 
I think they're more likely than not to completely miss this line.  On the other
hand, if you pass in the registry to your constructor (along with the pref
store) than they're more likely to notice it.

I see both as a very flake-prone (non-robust? way of trying to prevent
regressions, so my opinion is that either:
1)  Breakages here aren't a big concern.
Or
2)  They are a real concern, and this method is too easy to miss, so is not
robust enough to protect against them.

If this is a big concern, we could make a wrapper around the PrefStore, I
suppose, where when you register for a message, you get a unique object to
access it, but that seems like overkill.

[Bernhard Bauer, 2015/02/04 21:48:00]

The difference is that a PrefService requires a preference to be registered
beforehand, so if someone forgets it, they will get a DCHECK when trying to
access the preference. PrefStore does not have that requirement.
Yeah, PrefRegistry is desigend for use with a full PrefService and not just a
PrefStore. Really, if we want to use PrefRegistry, we could just directly use a
PrefService and load it asynchronously. We would still have to deal with storing
in-memory data before it is loaded, but we could probably do that with _another_
PrefService that has an in-memory user pref store...

The alternative would be to just roll our own registration mechanism, and ensure
accesses to the contents of the PrefStore always go through that (which we could
then possibly even combine with the ScopedPrefNotifier...). But TBH, I'm not
sure collisions are even going to be that likely, assuming that there are only
going to be handful of clients and people use sensible preference keys.

+
+  // Set up a persistent store for use by the network stack on the IO thread.
+  base::FilePath network_json_store_filepath(
+      profile_path_.Append(chrome::kNetworkPersistentStateFilename));
+  network_json_store_ = new JsonPrefStore(
+      network_json_store_filepath,
+      JsonPrefStore::GetTaskRunnerForFile(network_json_store_filepath,
+                                          BrowserThread::GetBlockingPool()),
+      scoped_ptr<PrefFilter>());
+  network_json_store_->ReadPrefsAsync(nullptr);
+
   net::URLRequestContext* main_context = main_request_context();
 
   IOThread* const io_thread = profile_params->io_thread;
@@ -611,6 +630,7 @@ void ProfileImplIOData::InitializeInternal(
   sdch_manager_.reset(new net::SdchManager);
   sdch_policy_.reset(new net::SdchOwner(sdch_manager_.get(), main_context));
   main_context->set_sdch_manager(sdch_manager_.get());
+  sdch_policy_->EnablePersistentStorage(network_json_store_.get());
 
   // Create a media request context based on the main context, but using a
   // media cache.  It shares the same job factory as the main context.