[NTP Snippets] Unschedule fetches when the service should be disabled

components/ntp_snippets/ntp_snippets_service.h

Index: components/ntp_snippets/ntp_snippets_service.h
diff --git a/components/ntp_snippets/ntp_snippets_service.h b/components/ntp_snippets/ntp_snippets_service.h
index a5bac53c11f3fe8e0bd2034c6e06dcb037ef7eed..230c5e90899f0881b6ac19bc299ae283a41eef3e 100644
--- a/components/ntp_snippets/ntp_snippets_service.h
+++ b/components/ntp_snippets/ntp_snippets_service.h
@@ -16,7 +16,6 @@
 #include "base/macros.h"
 #include "base/observer_list.h"
 #include "base/scoped_observer.h"
-#include "base/sequenced_task_runner.h"
 #include "base/timer/timer.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/ntp_snippets/ntp_snippet.h"
@@ -50,6 +49,60 @@ class SyncService;
 
 namespace ntp_snippets {
 
+enum class DisabledReason {

[Marc Treib, 2016/05/25 14:47:32]

I'd prefer to keep this and State in the NTPSnippetsService class, since they're
not really useful outside of it (and aren't even part of the public interface,
right?)

[dgn, 2016/06/03 19:02:24]

This is going to be used to provide a way for Java to know what's wrong and
display the right information to the user. I moved state back in the class.

+  // Snippets are enabled
+  NONE,
+  // Snippets have been disabled as part of the service configuration.
+  EXPLICITLY_DISABLED,
+  // History sync is not enabled, and the service requires it to be enabled.
+  HISTORY_SYNC_DISABLED,
+
+  HISTORY_SYNC_STATE_UNKNOWN
+};
+
+// Possible state transitions:
+//           NOT_INITED ------+
+//               v            |
+//  +---- ---- INITED         |
+//  |        /       \        |
+//  |   READY <--> DISABLED <-+
+//  |       \        /
+//  +-----> SHUT_DOWN
+enum class State {
+  // The service has just been created. Can change to states:
+  // - DISABLED if Init(false) was called. In that case the service will stay
+  //   disabled until it is shut down.
+  // - INITED otherwise
+  NOT_INITED,

[Marc Treib, 2016/05/25 14:47:32]

This state was just removed in https://codereview.chromium.org/1997473004

[dgn, 2016/06/03 19:02:24]

Left that one in, and removed INITED. the constructor starts the DB load, and
when it finishes it now moves to READY.

+
+  // The service passed the initialization, and is now loading snippets from
+  // the local database. Can change to states:
+  // - READY: if GetStateForDependenciesStatus returns it.
+  // - DISABLED: if GetStateForDependenciesStatus returns it.
+  INITED,
+
+  // The service registered observers, timers, etc. and is ready to answer to
+  // queries, fetch snippets... Can change to states:
+  // - READY: noop
+  // - DISABLED: if the state changed, for example after |OnStateChanged| is
+  //             called
+  // - SHUT_DOWN: is |Shutdown| is called, during the browser shutdown.
+  READY,

[Marc Treib, 2016/05/30 16:26:15]

FYI: Based on Bernhard's comments on my CL, it looks like the INITED vs READY
distinction will go away again soon. That should simplify things here a bit :)

[dgn, 2016/06/03 19:02:24]

Hum... I ran in another weird case. Will comment that on the next patch set.

+
+  // The service is disabled and unregistered the related resources.
+  // Can change to states:
+  // - DISABLED: noop
+  // - READY: if the state changed, for example after |OnStateChanged| is
+  //             called
+  // - SHUT_DOWN: is |Shutdown| is called, during the browser shutdown.
+  DISABLED,
+
+  // The service shutdown and can't be used anymore. This state is checked for
+  // early exit in callbacks from observers.
+  SHUT_DOWN
+};
+
+class NTPSnippetsDatabase;
 class NTPSnippetsServiceObserver;
 
 // Stores and vends fresh content data for the NTP.
@@ -63,15 +116,14 @@ class NTPSnippetsService : public KeyedService,
   // 'en' or 'en-US'. Note that this code should only specify the language, not
   // the locale, so 'en_US' (English language with US locale) and 'en-GB_US'
   // (British English person in the US) are not language codes.
-  NTPSnippetsService(
-      PrefService* pref_service,
-      sync_driver::SyncService* sync_service,
-      suggestions::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<image_fetcher::ImageFetcher> image_fetcher);
+  NTPSnippetsService(PrefService* pref_service,
+                     sync_driver::SyncService* sync_service,
+                     suggestions::SuggestionsService* suggestions_service,
+                     const std::string& application_language_code,
+                     NTPSnippetsScheduler* scheduler,
+                     std::unique_ptr<NTPSnippetsFetcher> snippets_fetcher,
+                     std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher,
+                     std::unique_ptr<NTPSnippetsDatabase> database);
   ~NTPSnippetsService() override;
 
   static void RegisterProfilePrefs(PrefRegistrySimple* registry);
@@ -81,6 +133,11 @@ class NTPSnippetsService : public KeyedService,
   // Inherited from KeyedService.
   void Shutdown() override;
 
+  // Returns whether the service is ready. While this is false, the list of
+  // snippets will be empty, and all modifications to it (fetch, discard, etc)
+  // will be ignored.
+  bool ready() const { return state_ == State::READY; }
+
   // Fetches snippets from the server and adds them to the current ones.
   void FetchSnippets();
   // Fetches snippets from the server for specified hosts (overriding
@@ -127,6 +184,10 @@ class NTPSnippetsService : public KeyedService,
   void AddObserver(NTPSnippetsServiceObserver* observer);
   void RemoveObserver(NTPSnippetsServiceObserver* observer);
 
+  // Returns a reason why the service could be disabled, or DisabledReason::NONE
+  // if it's not.
+  DisabledReason GetDisabledReason();
+
   // Returns the maximum number of snippets that will be shown at once.
   static int GetMaxSnippetCountForTesting();
 
@@ -139,36 +200,45 @@ class NTPSnippetsService : public KeyedService,
   // sync_driver::SyncServiceObserver implementation.
   void OnStateChanged() override;
 
+  // Callback for the NTPSnippetsDatabase.
+  void OnDatabaseLoaded(NTPSnippet::PtrVector snippets);
+
+  // Callback for the SuggestionsService.
   void OnSuggestionsChanged(const suggestions::SuggestionsProfile& suggestions);
+
+  // Callback for the NTPSnippetsFetcher.
   void OnFetchFinished(NTPSnippetsFetcher::OptionalSnippets snippets);
 
   // Merges newly available snippets with the previously available list.
   void MergeSnippets(NTPSnippet::PtrVector new_snippets);
-  // TODO(treib): Investigate a better storage, maybe LevelDB or SQLite?
-  void LoadSnippetsFromPrefs();
-  void StoreSnippetsToPrefs();
-
-  void LoadDiscardedSnippetsFromPrefs();
-  void StoreDiscardedSnippetsToPrefs();
 
   std::set<std::string> GetSnippetHostsFromPrefs() const;
   void StoreSnippetHostsToPrefs(const std::set<std::string>& hosts);
 
   void LoadingSnippetsFinished();
 
-  // Checks whether the state of the sync service is incompatible with showing
-  // snippets. We need history sync to be active.
-  // Note: The state is considered compatible if the service is still
-  // initializing and the sync state is not known.
-  bool IsSyncStateIncompatible();
+  // Returns whether the service should be enabled or disabled depending on its
+  // internal state and the state of its dependencies.
+  State GetStateForDependenciesStatus();

[dgn, 2016/05/24 18:13:30]

I'm thinking about moving all the service's state management methods and members
to a different class, wdyt?

That would be the new ones below, + add an interface with state change events.
Not sure if it would be worth the trouble, but it would remove some stuff from
this class that is starting to get quite big.

[Marc Treib, 2016/05/25 14:47:32]

Hm, seems like it would be hard to cleanly separate that? Not sure if it's worth
the trouble (and extra indirections), but I'm not opposed if you want to try it.
I'd prefer a follow-up CL though.

+
+  // Applies state transitions (see |State|'s documentation )and verifies them.
+  void EnterState(State state);
+
+  // Enable the service. Do not call directly, use |EnterState| instead.
+  void Enable();
 
-  enum class State {
-    NOT_INITED,
-    INITED,
-    SHUT_DOWN
-  } state_;
+  // Disable the service. Do not call directly, use |EnterState| instead.
+  void Disable();
 
-  bool enabled_;
+  // Load snippets from the DB. Do not call directly, use |EnterState| instead/
+  void LoadFromDB();
+
+  State state_;
+
+  // The service was set up to be disabled and should not be enabled by any
+  // state change. We track this to make sure we clear scheduled tasks, which
+  // persist even when Chrome is stopped.
+  bool explicitly_disabled_;
 
   PrefService* pref_service_;
 
@@ -182,9 +252,6 @@ class NTPSnippetsService : public KeyedService,
 
   suggestions::SuggestionsService* suggestions_service_;
 
-  // The SequencedTaskRunner on which file system operations will be run.
-  scoped_refptr<base::SequencedTaskRunner> file_task_runner_;
-
   // All current suggestions (i.e. not discarded ones).
   NTPSnippet::PtrVector snippets_;
 
@@ -216,6 +283,9 @@ class NTPSnippetsService : public KeyedService,
 
   std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher_;
 
+  // The database for persisting snippets.
+  std::unique_ptr<NTPSnippetsDatabase> database_;
+
   DISALLOW_COPY_AND_ASSIGN(NTPSnippetsService);
 };