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

components/ntp_snippets/ntp_snippets_service.h

 // 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.
 
 #ifndef COMPONENTS_NTP_SNIPPETS_NTP_SNIPPETS_SERVICE_H_
 #define COMPONENTS_NTP_SNIPPETS_NTP_SNIPPETS_SERVICE_H_
 
 #include <stddef.h>
 
 #include <memory>
@@ skipping 31 matching lines @@
 namespace suggestions {
 class SuggestionsProfile;
 }
 
 namespace sync_driver {
 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;
 
 // Stores and vends fresh content data for the NTP.
 class NTPSnippetsService : public KeyedService,
                            public sync_driver::SyncServiceObserver {
  public:
   using ImageFetchedCallback =
       base::Callback<void(const std::string& snippet_id, const gfx::Image&)>;
 
@@ skipping 10 matching lines @@
                      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);
 
   // 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();
   // Fetches snippets from the server for specified hosts (overriding
   // suggestions from the suggestion service) and adds them to the current ones.
   // Only called from chrome://snippets-internals, DO NOT USE otherwise!
   // Ignored while |loaded()| is false.
   void FetchSnippetsFromHosts(const std::set<std::string>& hosts);
 
   // Available snippets.
@@ skipping 29 matching lines @@
   // Clears the lists of snippets previously discarded by the user.
   void ClearDiscardedSnippets();
 
   // Returns the lists of suggestion hosts the snippets are restricted to.
   std::set<std::string> GetSuggestionsHosts() const;
 
   // Observer accessors.
   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();
 
  private:
   FRIEND_TEST_ALL_PREFIXES(NTPSnippetsServiceWithSyncTest,
                            SyncStateCompatibility);
   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;
 
   // 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);
 
   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();
+
+  // 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_;
+  State 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_;
+  // 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_;
 
   sync_driver::SyncService* sync_service_;
 
   // The observer for the SyncService. When the sync state changes,
   // SyncService will call |OnStateChanged|, which is propagated to the
   // snippet observers.
   ScopedObserver<sync_driver::SyncService, sync_driver::SyncServiceObserver>
       sync_service_observer_;
@@ skipping 51 matching lines @@
   // or because a requirement (e.g. History Sync) is not fulfilled anymore.
   virtual void NTPSnippetsServiceDisabled() = 0;
 
  protected:
   virtual ~NTPSnippetsServiceObserver() {}
 };
 
 }  // namespace ntp_snippets
 
 #endif  // COMPONENTS_NTP_SNIPPETS_NTP_SNIPPETS_SERVICE_H_