Various NTPSnippetsService cleanups:

components/ntp_snippets/ntp_snippets_service.cc

Index: components/ntp_snippets/ntp_snippets_service.cc
diff --git a/components/ntp_snippets/ntp_snippets_service.cc b/components/ntp_snippets/ntp_snippets_service.cc
index 02524508ceca77bf9620436e54bc6604c47ab25e..0e4a7931fecf57925a65360b746782b9f2a2862c 100644
--- a/components/ntp_snippets/ntp_snippets_service.cc
+++ b/components/ntp_snippets/ntp_snippets_service.cc
@@ -126,7 +126,7 @@ std::set<std::string> GetSuggestionsHostsImpl(
 }
 
 std::unique_ptr<base::ListValue> SnippetsToListValue(
-    const NTPSnippetsService::NTPSnippetStorage& snippets) {
+    const NTPSnippet::PtrVector& snippets) {
   std::unique_ptr<base::ListValue> list(new base::ListValue);
   for (const auto& snippet : snippets) {
     std::unique_ptr<base::DictionaryValue> dict = snippet->ToDictionary();
@@ -135,7 +135,7 @@ std::unique_ptr<base::ListValue> SnippetsToListValue(
   return list;
 }
 
-bool ContainsSnippet(const NTPSnippetsService::NTPSnippetStorage& haystack,
+bool ContainsSnippet(const NTPSnippet::PtrVector& haystack,
                      const std::unique_ptr<NTPSnippet>& needle) {
   const std::string& id = needle->id();
   return std::find_if(haystack.begin(), haystack.end(),
@@ -161,7 +161,9 @@ NTPSnippetsService::NTPSnippetsService(
     NTPSnippetsScheduler* scheduler,
     std::unique_ptr<NTPSnippetsFetcher> snippets_fetcher,
     std::unique_ptr<ImageFetcher> image_fetcher)
-    : enabled_(false),
+    : did_init_(false),
+      did_shutdown_(false),
+      enabled_(false),
       pref_service_(pref_service),
       suggestions_service_(suggestions_service),
       file_task_runner_(file_task_runner),
@@ -173,7 +175,9 @@ NTPSnippetsService::NTPSnippetsService(
       &NTPSnippetsService::OnFetchFinished, base::Unretained(this)));
 }
 
-NTPSnippetsService::~NTPSnippetsService() {}
+NTPSnippetsService::~NTPSnippetsService() {
+  DCHECK(!did_init_ || did_shutdown_);
+}
 
 // static
 void NTPSnippetsService::RegisterProfilePrefs(PrefRegistrySimple* registry) {
@@ -183,6 +187,9 @@ void NTPSnippetsService::RegisterProfilePrefs(PrefRegistrySimple* registry) {
 }
 
 void NTPSnippetsService::Init(bool enabled) {
+  DCHECK(!did_init_);
+  did_init_ = true;
+
   enabled_ = enabled;
   if (enabled_) {
     // |suggestions_service_| can be null in tests.
@@ -205,8 +212,14 @@ void NTPSnippetsService::Init(bool enabled) {
 }
 
 void NTPSnippetsService::Shutdown() {
+  if (!did_init_)

[Bernhard Bauer, 2016/05/13 16:04:05]

Hm... under which circumstances would we shut down a service that has never been
initialized?

[Marc Treib, 2016/05/17 08:38:14]

Shutdown is overridden from BrowserContextKeyedService, but Init isn't, so we
shouldn't expect an exact 1:1 correspondence. If for some reason, the browser
got torn down before Init was called, there might still be a Shutdown call.
That said, I'm not completely sure if this actually happens in practice - I
copied the exact checks from SupervisedUserService, assuming that whatever's
there would be reasonable ;)

[Bernhard Bauer, 2016/05/17 09:23:40]

Aww, I thought you would have learned by now 😉 Somewhat more srsly,
SupervisedUserService is not a great example, because it has to be initialized
at a specific point very early during profile loading (so that the profile is
correctly marked as supervised, I think).

I would be tempted to make this a DCHECK just so that we learn if this can
actually happen (and I think it can't).

Alternatively, we should be able to mark the factory as
ServiceIsCreatedWithBrowserContext(), so that it *will* in fact be tied to
profile creation more directly. (We can figure out the value of |enabled| in
here instead of passing it in.)

[Marc Treib, 2016/05/17 11:21:18]

Alright, done.
(Though really, we only care that Init isn't called twice. So if the !inited
case does happen here, we'll probably just remove the DCHECK.)
We could, but why? We don't really care when exactly the service is created. I
suppose "at profile creation" is as good as any other time, but presumably
there's a reason why that isn't the default?

[Bernhard Bauer, 2016/05/17 12:38:13]

The default is to create services lazily when they're accessed. That is a good
default IMO, but we want to schedule refetching when we start.

Mostly the reason why I would like to do that is to move stuff out of
ProfileManager, which is starting to become this grab bag for random stuff to
happen at profile creation time. (Not for this CL though.)

+    return;
+  DCHECK(!did_shutdown_);
+  did_shutdown_ = true;
+
   FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
                     NTPSnippetsServiceShutdown());
+  suggestions_service_subscription_.reset();
   enabled_ = false;
 }
 
@@ -350,7 +363,7 @@ void NTPSnippetsService::OnFetchFinished(
   LoadingSnippetsFinished();
 }
 
-void NTPSnippetsService::MergeSnippets(NTPSnippetStorage new_snippets) {
+void NTPSnippetsService::MergeSnippets(NTPSnippet::PtrVector new_snippets) {
   // Remove new snippets that we already have, or that have been discarded.
   new_snippets.erase(
       std::remove_if(new_snippets.begin(), new_snippets.end(),
@@ -400,7 +413,7 @@ void NTPSnippetsService::MergeSnippets(NTPSnippetStorage new_snippets) {
 }
 
 void NTPSnippetsService::LoadSnippetsFromPrefs() {
-  NTPSnippetStorage prefs_snippets;
+  NTPSnippet::PtrVector prefs_snippets;
   bool success = NTPSnippet::AddFromListValue(
       *pref_service_->GetList(prefs::kSnippets), &prefs_snippets);
   DCHECK(success) << "Failed to parse snippets from prefs";