Various NTPSnippetsService cleanups:

components/ntp_snippets/ntp_snippets_service.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/ntp_snippets/ntp_snippets_service.h"
 
 #include <algorithm>
 #include <iterator>
 #include <utility>
 
@@ skipping 108 matching lines @@
   for (int i = 0; i < suggestions.suggestions_size(); ++i) {
     const ChromeSuggestion& suggestion = suggestions.suggestions(i);
     GURL url(suggestion.url());
     if (url.is_valid())
       hosts.insert(url.host());
   }
   return hosts;
 }
 
 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();
     list->Append(std::move(dict));
   }
   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(),
                       [&id](const std::unique_ptr<NTPSnippet>& snippet) {
                         return snippet->id() == id;
                       }) != haystack.end();
 }
 
 void WrapImageFetchedCallback(
     const NTPSnippetsService::ImageFetchedCallback& callback,
     const GURL& snippet_id_url,
     const SkBitmap* bitmap) {
   callback.Run(snippet_id_url.spec(), bitmap);
 }
 
 }  // namespace
 
 NTPSnippetsService::NTPSnippetsService(
     PrefService* pref_service,
     SuggestionsService* suggestions_service,
     scoped_refptr<base::SequencedTaskRunner> file_task_runner,
     const std::string& application_language_code,
     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),
       application_language_code_(application_language_code),
       scheduler_(scheduler),
       snippets_fetcher_(std::move(snippets_fetcher)),
       image_fetcher_(std::move(image_fetcher)) {
   snippets_fetcher_->SetCallback(base::Bind(
       &NTPSnippetsService::OnFetchFinished, base::Unretained(this)));
 }
 
-NTPSnippetsService::~NTPSnippetsService() {}
+NTPSnippetsService::~NTPSnippetsService() {
+  DCHECK(!did_init_ || did_shutdown_);
+}
 
 // static
 void NTPSnippetsService::RegisterProfilePrefs(PrefRegistrySimple* registry) {
   registry->RegisterListPref(prefs::kSnippets);
   registry->RegisterListPref(prefs::kDiscardedSnippets);
   registry->RegisterListPref(prefs::kSnippetHosts);
 }
 
 void NTPSnippetsService::Init(bool enabled) {
+  DCHECK(!did_init_);
+  did_init_ = true;
+
   enabled_ = enabled;
   if (enabled_) {
     // |suggestions_service_| can be null in tests.
     if (suggestions_service_) {
       suggestions_service_subscription_ = suggestions_service_->AddCallback(
           base::Bind(&NTPSnippetsService::OnSuggestionsChanged,
                      base::Unretained(this)));
     }
 
     // Get any existing snippets immediately from prefs.
     LoadDiscardedSnippetsFromPrefs();
     LoadSnippetsFromPrefs();
 
     // If we don't have any snippets yet, start a fetch.
     if (snippets_.empty())
       FetchSnippets();
   }
 
   RescheduleFetching();
 }
 
 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;
 }
 
 void NTPSnippetsService::FetchSnippets() {
   FetchSnippetsFromHosts(GetSuggestionsHosts());
 }
 
 void NTPSnippetsService::FetchSnippetsFromHosts(
     const std::set<std::string>& hosts) {
   snippets_fetcher_->FetchSnippetsFromHosts(hosts, application_language_code_,
@@ skipping 123 matching lines @@
     // Sparse histogram used because the number of snippets is small (bound by
     // kMaxSnippetCount).
     DCHECK_LE(snippets->size(), static_cast<size_t>(kMaxSnippetCount));
     UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.NumArticlesFetched",
                                 snippets->size());
     MergeSnippets(std::move(*snippets));
   }
   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(),
                      [this](const std::unique_ptr<NTPSnippet>& snippet) {
                        return ContainsSnippet(discarded_snippets_, snippet) ||
                               ContainsSnippet(snippets_, snippet);
                      }),
       new_snippets.end());
 
   // Fill in default publish/expiry dates where required.
@@ skipping 29 matching lines @@
     }
   }
 
   // Insert the new snippets at the front.
   snippets_.insert(snippets_.begin(),
                    std::make_move_iterator(new_snippets.begin()),
                    std::make_move_iterator(new_snippets.end()));
 }
 
 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";
   MergeSnippets(std::move(prefs_snippets));
   LoadingSnippetsFinished();
 }
 
 void NTPSnippetsService::StoreSnippetsToPrefs() {
   pref_service_->Set(prefs::kSnippets, *SnippetsToListValue(snippets_));
 }
@@ skipping 79 matching lines @@
     if (snippet->expiry_date() < next_expiry)
       next_expiry = snippet->expiry_date();
   }
   DCHECK_GT(next_expiry, expiry);
   expiry_timer_.Start(FROM_HERE, next_expiry - expiry,
                       base::Bind(&NTPSnippetsService::LoadingSnippetsFinished,
                                  base::Unretained(this)));
 }
 
 }  // namespace ntp_snippets