[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>
 #include <set>
 #include <string>
 #include <vector>
 
 #include "base/gtest_prod_util.h"
 #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"
 #include "components/ntp_snippets/ntp_snippets_fetcher.h"
 #include "components/ntp_snippets/ntp_snippets_scheduler.h"
 #include "components/suggestions/suggestions_service.h"
 #include "components/sync_driver/sync_service_observer.h"
 
 class PrefRegistrySimple;
 class PrefService;
@@ skipping 13 matching lines @@
 namespace suggestions {
 class SuggestionsProfile;
 }
 
 namespace sync_driver {
 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.
 class NTPSnippetsService : public KeyedService,
                            public sync_driver::SyncServiceObserver {
  public:
   using ImageFetchedCallback =
       base::Callback<void(const std::string& snippet_id, const gfx::Image&)>;
 
   // |application_language_code| should be a ISO 639-1 compliant string, e.g.
   // '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);
 
   void Init(bool enabled);
 
   // 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
   // suggestions from the suggestion service) and adds them to the current ones.
   void FetchSnippetsFromHosts(const std::set<std::string>& hosts);
 
   // Available snippets.
   const NTPSnippet::PtrVector& snippets() const { return snippets_; }
 
   // Returns the list of snippets previously discarded by the user (that are
@@ skipping 26 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
+  // if it's not.
+  DisabledReason GetDisabledReason();
+
   // 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);
 
   // 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.

 
-  enum class State {
-    NOT_INITED,
-    INITED,
-    SHUT_DOWN
-  } state_;
+  // Applies state transitions (see |State|'s documentation )and verifies them.
+  void EnterState(State state);
 
-  bool enabled_;
+  // Enable the service. Do not call directly, use |EnterState| instead.
+  void Enable();
+
+  // Disable the service. Do not call directly, use |EnterState| instead.
+  void Disable();
+
+  // 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_;
 
   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_;
 
   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_;
 
   // Suggestions that the user discarded. We keep these around until they expire
   // so we won't re-add them on the next fetch.
   NTPSnippet::PtrVector discarded_snippets_;
 
   // The ISO 639-1 code of the language used by the application.
   const std::string application_language_code_;
 
@@ skipping 11 matching lines @@
   std::unique_ptr<SuggestionsSubscription> suggestions_service_subscription_;
 
   // The snippets fetcher.
   std::unique_ptr<NTPSnippetsFetcher> snippets_fetcher_;
 
   // Timer that calls us back when the next snippet expires.
   base::OneShotTimer expiry_timer_;
 
   std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher_;
 
+  // The database for persisting snippets.
+  std::unique_ptr<NTPSnippetsDatabase> database_;
+
   DISALLOW_COPY_AND_ASSIGN(NTPSnippetsService);
 };
 
 class NTPSnippetsServiceObserver {
  public:
   // Sent every time the service loads a new set of data.
   virtual void NTPSnippetsServiceLoaded() = 0;
   // Sent when the service is shutting down.
   virtual void NTPSnippetsServiceShutdown() = 0;
   // Sent when the service has been disabled. Can be from explicit user action
   // 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_