📰 The Status card reports disabled sync states

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/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;
+class SigninManagerBase;
 
 namespace base {
 class Value;
 }
 
 namespace gfx {
 class Image;
 }
 
 namespace image_fetcher {
 class ImageFetcher;
 }
 
 namespace suggestions {
 class SuggestionsProfile;
 }
 
 namespace sync_driver {
 class SyncService;
 }
 
 namespace ntp_snippets {
 
-enum class DisabledReason {
+// A Java counterpart will be generated for this enum.
+// GENERATED_JAVA_ENUM_PACKAGE: org.chromium.chrome.browser.ntp.snippets

[noyau (Ping after 24h), 2016/06/21 14:28:55]

This is only true for android, please amend the comment to make this obvious. I
would argue that this comment should not be here in the first place. Is this
some kind of automated process generating this java code?

[dgn, 2016/06/21 15:47:39]

Yes, the java_cpp_enum I added to the BUILD.gn file does that. It runs only on
android builds.

+enum class DisabledReason : int {
   // Snippets are enabled
   NONE,
   // Snippets have been disabled as part of the service configuration.
   EXPLICITLY_DISABLED,
+  // The user is not signed in, and the service requires it to be enabled.
+  SIGNED_OUT,
+  // Sync is not enabled, and the service requires it to be enabled.
+  SYNC_DISABLED,
+  // The service requires passphrase encryption to be disabled.
+  PASSPHRASE_ENCRYPTION_ENABLED,

[noyau (Ping after 24h), 2016/06/21 14:28:55]

To be honest I don't like the idea of adding to this class a dependency on
signin. I was already reluctant when there was a dependency on sync, this makes
it too tightly coupled for my taste.

All this disabling  logic could be handled in a separate class. As it is coded
now I feel this is just increasing coupling for a very small gain codewise. 

Ideally the service should be limited to being a service storing and vending
snippets. The fetcher is the one getting the data to feed into the service. Can
you move this enabling/disabling code somewhere else? This would make the tests
clearer as well as of now I'm having difficulties figuring out which of those
disabled mode are tested.

[dgn, 2016/06/21 15:47:39]

Very valid concern. We are working on some refactoring to the Java/XML side to
better prepare supporting different types of content. Some similar effort should
be made here too, in a way that would be compatible with offline pages for
example, that I think we started working on for Android.

So would it be okay to land the disabling as it is, and I'll send you a design
doc about the refactor and how the component would be with regards to new data
sources?

[dgn, 2016/06/24 20:25:20]

Moved the dependencies to NTPSnippetStatusService. That will be updated again
when markusheintz@'s refactor of the component happens.

[markusheintz_, 2016/06/28 14:22:53]

I agree with Eric that we need to split this into multiple pieces.
I will discuss this with all involved parties.

   // 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&)>;
 
   // |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(bool enabled,
                      PrefService* pref_service,
+                     SigninManagerBase* signin_manager,
                      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);
@@ skipping 24 matching lines @@
   // Returns the list of snippets previously discarded by the user (that are
   // not expired yet).
   const NTPSnippet::PtrVector& discarded_snippets() const {
     return discarded_snippets_;
   }
 
   const NTPSnippetsFetcher* snippets_fetcher() const {
     return snippets_fetcher_.get();
   }
 
+  // Returns a reason why the service is disabled, or DisabledReason::NONE
+  // if it's not.
+  DisabledReason disabled_reason() const { return disabled_reason_; }
+
   // (Re)schedules the periodic fetching of snippets. This is necessary because
   // the schedule depends on the time of day.
   void RescheduleFetching();
 
   // Fetches the image for the snippet with the given |snippet_id| and runs the
   // |callback|. If that snippet doesn't exist or the fetch fails, the callback
   // gets a null image.
   void FetchSnippetImage(const std::string& snippet_id,
                          const ImageFetchedCallback& callback);
 
   // Deletes all currently stored snippets.
   void ClearSnippets();
 
   // Discards the snippet with the given |snippet_id|, if it exists. Returns
   // true iff a snippet was discarded.
   bool DiscardSnippet(const std::string& snippet_id);
 
   // 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() const;
-
   // Returns the maximum number of snippets that will be shown at once.
   static int GetMaxSnippetCountForTesting();
 
  private:
   FRIEND_TEST_ALL_PREFIXES(NTPSnippetsServiceTest, SyncStateCompatibility);
   FRIEND_TEST_ALL_PREFIXES(NTPSnippetsServiceTest, HistorySyncStateChanges);
 
   // Possible state transitions:
   //  +------- NOT_INITED ------+
   //  |        /       \        |
@@ skipping 51 matching lines @@
   // Removes the expired snippets (including discarded) from the service and the
   // database, and schedules another pass for the next expiration.
   void ClearExpiredSnippets();
 
   // Completes the initialization phase of the service, registering the last
   // observers. This is done after construction, once the database is loaded.
   void FinishInitialization();
 
   void LoadingSnippetsFinished();
 
+  // Updates |disabled_reason_| (see |GetNewDisabledReason()|) and notifies
+  // observers of changes if necessary.
+  void UpdateDisabledReason();
+
+  // Utility method that looks at the state of the service and its dependencies
+  // to determine if and why it should be disabled.
+  DisabledReason GetNewDisabledReason() const;
+
   // Returns whether the service should be enabled or disable depending on its
   // internal state and the state of its dependencies.
   State GetStateForDependenciesStatus();
 
   // Verifies state transitions (see |State|'s documentation) and applies them.
   // Does nothing if called with the current state.
   void EnterState(State state);
 
   // Enables the service and triggers a fetch if required. Do not call directly,
   // use |EnterState| instead.
   void EnterStateEnabled(bool fetch_snippets);
 
   // Disables the service. Do not call directly, use |EnterState| instead.
   void EnterStateDisabled();
 
   // Applies the effects of the transition to the SHUT_DOWN state. Do not call
   // directly, use |EnterState| instead.
   void EnterStateShutdown();
 
   void ClearDeprecatedPrefs();
 
   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_;
+  DisabledReason disabled_reason_;
 
   PrefService* pref_service_;
 
+  SigninManagerBase* signin_manager_;
+
   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_;
 
@@ skipping 37 matching lines @@
 
   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;
+  // Sent when the state of the service is changing. Something changed in its
+  // dependencies so it's notifying observers about incoming data changes.
+  // If the service might be enabled, DisabledReason::NONE will be provided.
+  virtual void NTPSnippetsServiceDisabledReasonChanged(DisabledReason) = 0;
 
  protected:
   virtual ~NTPSnippetsServiceObserver() {}
 };
 
 }  // namespace ntp_snippets
 
 #endif  // COMPONENTS_NTP_SNIPPETS_NTP_SNIPPETS_SERVICE_H_