[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 f6f6ebe8c7f2cfb008fc0aababc3c5b140976377..353722b4c7a5eadf66204406556b137ff38c607b 100644
--- a/components/ntp_snippets/ntp_snippets_service.h
+++ b/components/ntp_snippets/ntp_snippets_service.h
@@ -49,6 +49,17 @@ class SyncService;
 
 namespace ntp_snippets {
 
+enum class DisabledReason {
+  // 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,
+  // The sync service is not completely initialized, and the status is unknown.
+  HISTORY_SYNC_STATE_UNKNOWN
+};
+
 class NTPSnippetsDatabase;
 class NTPSnippetsServiceObserver;
 
@@ -79,9 +90,14 @@ class NTPSnippetsService : public KeyedService,
   // Inherited from KeyedService.
   void Shutdown() override;
 
-  // Returns whether the initial set of snippets has been loaded from the
-  // database. While this is false, the list of snippets will be empty.
-  bool loaded() const { return state_ == State::LOADED; }
+  // 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; }
+
+  // Returns whether the service is initialized. While this is false, some
+  // calls may trigger DCHECKs.
+  bool initialized() const { return ready() || state_ == State::DISABLED; }
 
   // Fetches snippets from the server and adds them to the current ones.
   void FetchSnippets();
@@ -131,6 +147,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

[Marc Treib, 2016/06/06 08:38:33]

"could be disabled"? If this returns something !=NONE, then the service *is*
disabled, right?

[dgn, 2016/06/06 15:44:39]

It doesn't only depend on the state of the service itself, since we use it do
check whether we should disable the service. 
Using sync as an example, it looks at the sync state, and then says that the
service should be disabled because of the current sync state. In the sync
observer implementation we call it and it returns "DISABLED" even though the
service is currently enabled. And we use that result to transition the service
to DISABLED. 

I'm not sure what the best way to clarify this in the documentation would be...

[Marc Treib, 2016/06/07 08:55:39]

Hm. So I guess the issue is that this method serves two purposes:
- Internal: "Should we be disabled?"
- External: "what's the current disable reason, if any?"

Maybe just something like "Returns the current disable reason, or
DisabledReason::NONE if there isn't one."? That seems to cover both cases.

+  // if it's not.
+  DisabledReason GetDisabledReason();

[Marc Treib, 2016/06/06 08:38:33]

const?

[dgn, 2016/06/06 15:44:39]

Done.

+
   // Returns the maximum number of snippets that will be shown at once.
   static int GetMaxSnippetCountForTesting();
 
@@ -140,6 +160,44 @@ class NTPSnippetsService : public KeyedService,
   FRIEND_TEST_ALL_PREFIXES(NTPSnippetsServiceWithSyncTest,
                            HistorySyncStateChanges);
 
+  // Possible state transitions:
+  //  +------- NOT_INITED ------+
+  //  |        /       \        |
+  //  |   READY <--> DISABLED <-+
+  //  |       \        /
+  //  +-----> SHUT_DOWN
+  enum class State {
+    // The service has just been created. Can change to states:
+    // - DISABLED: if the constructor was called with |enabled == false| . In
+    //             that case the service will stay disabled until it is shut
+    //             down. It can also enter this state after the database is
+    //             done loading and GetStateForDependenciesStatus identifies
+    //             the next state to be DISABLED.
+    // - READY: if GetStateForDependenciesStatus returns it, after the database
+    //          is done loading.
+    NOT_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

[Marc Treib, 2016/06/06 08:38:33]

What's "the state" here?

[dgn, 2016/06/06 15:44:39]

The state of other dependencies, like sync for example. Reworded.

+    //             called
+    // - SHUT_DOWN: is |Shutdown| is called, during the browser shutdown.

[Marc Treib, 2016/06/06 08:38:33]

s/is/if

[dgn, 2016/06/06 15:44:39]

Done.

+    READY,
+
+    // 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.

[Marc Treib, 2016/06/06 08:38:33]

here too

[dgn, 2016/06/06 15:44:39]

Done.

+    DISABLED,
+
+    // The service shutdown and can't be used anymore. This state is checked
+    // for early exit in callbacks from observers.
+    SHUT_DOWN
+  };
+
   // sync_driver::SyncServiceObserver implementation.
   void OnStateChanged() override;
 
@@ -160,24 +218,27 @@ class NTPSnippetsService : public KeyedService,
 
   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();
+
+  // Applies state transitions (see |State|'s documentation )and verifies them.

[Marc Treib, 2016/06/06 08:38:33]

nit: misplaced ")"
"Verifies and applies"? The verification should come first :)

[dgn, 2016/06/06 15:44:39]

Done.

+  void EnterState(State state);
+
+  // Enable the service. Do not call directly, use |EnterState| instead.

[Marc Treib, 2016/06/06 08:38:33]

nit: Enable*s* the service

[dgn, 2016/06/06 15:44:39]

Done.

+  void Enable();

[Marc Treib, 2016/06/06 08:38:33]

Maybe call this "EnterStateEnabled" or something like that? That would make it a
bit clearer IMO, and also make it kinda obvious that it's not supposed to be
called directly.

[dgn, 2016/06/06 15:44:39]

Done.

+
+  // Disable the service. Do not call directly, use |EnterState| instead.

[Marc Treib, 2016/06/06 08:38:33]

Disables

[dgn, 2016/06/06 15:44:39]

Done.

+  void Disable();
 
   void ClearDeprecatedPrefs();
 
-  enum class State {
-    INITED,    // Initial state before the DB has been loaded; no snippets yet.
-    LOADED,    // DB has been loaded and the service is ready for action.
-    SHUT_DOWN  // Shutdown has been called, service is inactive.
-  } state_;
-
-  // When |enabled_| is true the service will fetch snippets from the server
-  // using |snippets_fetcher_|, load snippets from device storage, and schedule
-  // the |scheduler_|.
-  bool enabled_;
+  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_;