[Remote suggestions] Do not fetch before EULA accepted

components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h

 // Copyright 2016 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_REMOTE_SCHEDULING_REMOTE_SUGGESTIONS_PROVIDER_H_
 #define COMPONENTS_NTP_SNIPPETS_REMOTE_SCHEDULING_REMOTE_SUGGESTIONS_PROVIDER_H_
 
 #include <memory>
 #include <set>
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/macros.h"
 #include "base/time/time.h"
 #include "components/ntp_snippets/content_suggestions_provider.h"
 #include "components/ntp_snippets/remote/persistent_scheduler.h"
 #include "components/ntp_snippets/remote/remote_suggestions_provider.h"
 #include "components/ntp_snippets/remote/remote_suggestions_scheduler.h"
 #include "components/ntp_snippets/remote/request_throttler.h"
+#include "components/web_resource/eula_accepted_notifier.h"
 
 class PrefRegistrySimple;
 class PrefService;
 
 namespace base {
 class Clock;
 }
 
 namespace ntp_snippets {
 
@@ skipping 19 matching lines @@
 // callback.
 // This class is final because it does things in its constructor which make it
 // unsafe to derive from it.
 // TODO(jkrcal): Introduce two-phase initialization and make the class not
 // final? (see the same comment for RemoteSuggestionsProvider)
 // TODO(jkrcal): Change the interface to ContentSuggestionsProvider. We do not
 // need any special functionality, all special should be exposed in the
 // Scheduler interface. crbug.com/695447
 class SchedulingRemoteSuggestionsProvider final
     : public RemoteSuggestionsProvider,
-      public RemoteSuggestionsScheduler {
+      public RemoteSuggestionsScheduler,
+      public web_resource::EulaAcceptedNotifier::Observer {

[tschumann, 2017/03/21 20:13:15]

as you know, I'm trying to avoid multiple inheritance.
In this case, we can actually quite nicely use aggregation as the observer is
using the RemoteSuggestionsScheduler's public interface.

So why not have a small class like EulaState with a constructor taking the
local_state_prefs and remote suggestions scheduler. EulaState would also
encapsulate the notifier (so no doubt if it's null or not)
The SchedulingRemoteSuggestionsProvider's constructor can instantiate build it
and simply ask that state for all info it needs. WDYT?

[jkrcal, 2017/03/22 09:36:04]

Done. WDYT?

[tschumann, 2017/03/22 09:47:01]

Thanks! The cyclic dependency is still a bit of a smell but I don't see and easy
way to improve this.

[jkrcal, 2017/03/22 16:48:01]

Acknowledged.

  public:
   SchedulingRemoteSuggestionsProvider(
-      Observer* observer,
+      ContentSuggestionsProvider::Observer* observer,
       std::unique_ptr<RemoteSuggestionsProvider> provider,
       PersistentScheduler* persistent_scheduler,
       const UserClassifier* user_classifier,
-      PrefService* pref_service,
+      PrefService* profile_prefs,
+      PrefService* local_state_prefs,
       std::unique_ptr<base::Clock> clock);
 
   ~SchedulingRemoteSuggestionsProvider() override;
 
   static void RegisterProfilePrefs(PrefRegistrySimple* registry);
 
   // RemoteSuggestionsScheduler implementation.
   void OnProviderActivated() override;
   void OnProviderDeactivated() override;
   void OnSuggestionsCleared() override;
@@ skipping 24 matching lines @@
       base::Time begin,
       base::Time end,
       const base::Callback<bool(const GURL& url)>& filter) override;
   void ClearCachedSuggestions(Category category) override;
   void OnSignInStateChanged() override;
   void GetDismissedSuggestionsForDebugging(
       Category category,
       const DismissedSuggestionsCallback& callback) override;
   void ClearDismissedSuggestionsForDebugging(Category category) override;
 
+  // EulaAcceptedNotifier::Observer implementation.
+  void OnEulaAccepted() override;
+
  private:
   // Abstract description of the fetching schedule.
   struct FetchingSchedule {
       static FetchingSchedule Empty();
       bool operator==(const FetchingSchedule& other) const;
       bool operator!=(const FetchingSchedule& other) const;
       bool is_empty() const;
 
       base::TimeDelta interval_persistent_wifi;
       base::TimeDelta interval_persistent_fallback;
@@ skipping 69 matching lines @@
   bool background_fetch_in_progress_;
 
   // Used to adapt the schedule based on usage activity of the user. Not owned.
   const UserClassifier* user_classifier_;
 
   // Request throttlers for limiting requests for different classes of users.
   RequestThrottler request_throttler_rare_ntp_user_;
   RequestThrottler request_throttler_active_ntp_user_;
   RequestThrottler request_throttler_active_suggestions_consumer_;
 
-  PrefService* pref_service_;
+  // We should not fetch in background before EULA gets accepted.
+  std::unique_ptr<web_resource::EulaAcceptedNotifier> eula_notifier_;
+
+  PrefService* profile_prefs_;
   std::unique_ptr<base::Clock> clock_;
   std::set<SchedulingRemoteSuggestionsProvider::TriggerType> enabled_triggers_;
 
   base::Time background_fetches_allowed_after_;
 
   DISALLOW_COPY_AND_ASSIGN(SchedulingRemoteSuggestionsProvider);
 };
 
 }  // namespace ntp_snippets
 
 #endif  // COMPONENTS_NTP_SNIPPETS_REMOTE_SCHEDULING_REMOTE_SUGGESTIONS_PROVIDER_H_