Snippets are enabled when search suggestions are enabled.

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 39 matching lines @@
 class NTPSnippetsService : public KeyedService {
  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,
       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);

[noyau (Ping after 24h), 2016/05/20 11:45:05]

7 arguments now. This starts to smell dirty.

|enabled| is a little unclear. There is no documentation for it, and I'm not
sure why it needs to be passed at creation time, can't is be deduced from the
other arguments? I'm not sure, as someone writing code to create a snippet
service, how I can make that decision. If I really want a disabled service I
would not create one in the first place no?

[Michael van Ouwerkerk, 2016/05/20 12:38:36]

I've added some documentation to the |enabled_| field, but I'm just doing
archaeology here, if anyone knows more please chime in!

The previous behavior had the |enabled| argument for the Init method, while the
constructor defaulted it to |false|. I think moving it to the constructor is not
worse, and moves logic from ProfileManager to the factory, which seems more
appropriate.

On iOS the service always had enabled = false. Also, the service is never
instantiated on iOS, afaict.

[Marc Treib, 2016/05/20 13:49:36]

Er, did you? Where?
I don't recall any particular reason for it being passed to Init rather than the
ctor, other than that the ctor didn't need it.

That said, it should be possible to just figure it out within the ctor itself,
so the logic doesn't need to be duplicated in the factories. If so, that would
IMO be nicer.
Yup, the service isn't used on iOS yet.

[Michael van Ouwerkerk, 2016/05/20 14:56:49]

Ah sorry, I didn't upload the docs before. Done now.

If I do a followup CL for observing pref changes, I'll see if enabled state be
can determined in the constructor. For the moment, it seems the factory is a
better place than ProfileManager.

[noyau (Ping after 24h), 2016/05/20 15:16:53]

Don't see it. Did you forget to upload?
Yes, that's my concern. Why is the service started in disabled mode by default?
That's why I'd like to understand this boolean. It should probably default to
true on iOS, or be tied to a flag.

As for not being used on iOS yet, you are right. But we are planning to use it
at some point, and I'd rather have working then :)

[Marc Treib, 2016/05/20 15:30:25]

We have a flag (in base::FeatureList) which should be used for that (plus the
pref that is being added in this CL).

[Marc Treib, 2016/05/20 15:30:25]

Aah, I missed the pref-dependent part of this; I was thinking only of the
FeatureList part which should be easy to figure out in the service itself.
So right now this will query the pref only once on startup, which is clearly
suboptimal, but okay if you want to do the dynamic part in another CL.

[Michael van Ouwerkerk, 2016/05/24 14:11:31]

Eric, there is another CL underway for changing how "enabled" works:
https://codereview.chromium.org/2000233002/

I would prefer to land this simple one as is, get the other one by Nicolas
landed as well, and then polish things some more after if still needed.

   ~NTPSnippetsService() override;
 
   static void RegisterProfilePrefs(PrefRegistrySimple* registry);
 
-  void Init(bool enabled);
+  void Init();
 
   // Inherited from KeyedService.
   void Shutdown() override;
 
   // 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);
 
@@ skipping 119 matching lines @@
   // hosts. (e.g. the user manually removed all MostLikely tiles.)
   virtual void NTPSnippetsServiceCleared() = 0;
 
  protected:
   virtual ~NTPSnippetsServiceObserver() {}
 };
 
 }  // namespace ntp_snippets
 
 #endif  // COMPONENTS_NTP_SNIPPETS_NTP_SNIPPETS_SERVICE_H_