[Remote suggestions] Do not fetch before EULA accepted

components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc

Index: components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc
diff --git a/components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc b/components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc
index b5ae799498a46492151a937672e36bef31a5edc7..c9a3716530218e577b9fe53482497e306f29ef4b 100644
--- a/components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc
+++ b/components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc
@@ -27,8 +27,10 @@
 #include "components/ntp_snippets/remote/test_utils.h"
 #include "components/ntp_snippets/status.h"
 #include "components/ntp_snippets/user_classifier.h"
+#include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/testing_pref_service.h"
 #include "components/variations/variations_params_manager.h"
+#include "components/web_resource/web_resource_pref_names.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -129,6 +131,10 @@ class SchedulingRemoteSuggestionsProviderTest
     SchedulingRemoteSuggestionsProvider::RegisterProfilePrefs(
         utils_.pref_service()->registry());
     RequestThrottler::RegisterProfilePrefs(utils_.pref_service()->registry());
+    // TODO(jkrcal) Create a static function in EulaAcceptedNotifier that
+    // registers this pref and replace the call in browser_process_impl.cc & in
+    // eula_accepted_notifier_unittest.cc with the new static function.
+    local_state_.registry()->RegisterBooleanPref(::prefs::kEulaAccepted, false);
     ResetProvider();
   }
 
@@ -146,7 +152,7 @@ class SchedulingRemoteSuggestionsProviderTest
         base::MakeUnique<SchedulingRemoteSuggestionsProvider>(
             /*observer=*/nullptr, std::move(underlying_provider),
             &persistent_scheduler_, &user_classifier_, utils_.pref_service(),
-            std::move(test_clock));
+            &local_state_, std::move(test_clock));
   }
 
   void SetVariationParameter(const std::string& param_name,
@@ -160,6 +166,16 @@ class SchedulingRemoteSuggestionsProviderTest
         {ntp_snippets::kArticleSuggestionsFeature.name});
   }
 
+  bool IsEulaNotifierAvailable() {
+    // Just check that the constructer does not return null unique_ptr. The
+    // notifier will be again desctructed right away.
+    return web_resource::EulaAcceptedNotifier::Create(&local_state_);

[tschumann, 2017/03/22 17:56:41]

let's make this more specific and add != nullptr
you can then reduce the comment to:
// Create() returns a unique_ptr, so this is no leak. 

[jkrcal, 2017/03/23 09:29:42]

Done.

+  }
+
+  void SetEulaAcceptedPref() {
+    local_state_.SetBoolean(::prefs::kEulaAccepted, true);
+  }
+
   // GMock cannot deal with move-only types. We need to pass the vector to the
   // mock function as const ref using this wrapper callback.
   void FetchDoneWrapper(
@@ -180,6 +196,11 @@ class SchedulingRemoteSuggestionsProviderTest
   base::SimpleTestClock* test_clock_;
 
   void ActivateUnderlyingProvider() {
+    SetEulaAcceptedPref();
+    scheduling_provider_->OnProviderActivated();
+  }
+
+  void ActivateUnderlyingProviderWithoutEula() {

[tschumann, 2017/03/22 17:56:41]

I'd get rid of this function as it's misleading. This function has no control
over whether Eula was accepted or not.

[jkrcal, 2017/03/23 09:29:43]

Done.

     scheduling_provider_->OnProviderActivated();
   }
 
@@ -190,6 +211,7 @@ class SchedulingRemoteSuggestionsProviderTest
  private:
   test::RemoteSuggestionsTestUtils utils_;
   UserClassifier user_classifier_;
+  TestingPrefServiceSimple local_state_;
 
   DISALLOW_COPY_AND_ASSIGN(SchedulingRemoteSuggestionsProviderTest);
 };
@@ -203,6 +225,58 @@ TEST_F(SchedulingRemoteSuggestionsProviderTest,
 }
 
 TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ShouldIgnoreEulaStateOnPlatformsWhereNotAvaiable) {
+  // Do not run the test if Eula notifier is available

[tschumann, 2017/03/22 17:56:41]

// Only run this tests on platforms that don't support Eula.
?

[jkrcal, 2017/03/23 09:29:43]

Done.

+  if (IsEulaNotifierAvailable()) {
+    return;
+  }
+
+  // First enable the scheduler.

[tschumann, 2017/03/22 17:56:41]

together with the comment about inlining
ActivateUnderlyingProviderWithoutEula().
// Activating the provider should schedule the background fetch.
EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
scheduling_provider_->OnProviderActivated();
// Verify fetches get triggered.
EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_));
scheduling_provider_->OnPersistentSchedulerWakeUp();

[jkrcal, 2017/03/23 09:29:42]

Done.

+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
+  ActivateUnderlyingProviderWithoutEula();
+
+  // A trigger results in a fetch even though Eula is not accepted (because it
+  // is not needed on this platform).
+  EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_));
+  scheduling_provider_->OnPersistentSchedulerWakeUp();
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ShouldIgnoreSignalsWhenEulaNotAccepted) {
+  // Do not run the test if Eula notifier is not available.

[tschumann, 2017/03/22 17:56:41]

// Only run this tests on platforms supporting Eula.

[jkrcal, 2017/03/23 09:29:42]

Done.

+  if (!IsEulaNotifierAvailable()) {
+    return;
+  }
+  // First enable the scheduler.

[tschumann, 2017/03/22 17:56:41]

// Activating the provider should schedule the background fetch.
?

[jkrcal, 2017/03/23 09:29:43]

Done.

+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
+  ActivateUnderlyingProviderWithoutEula();
+
+  // All signals are ignored because of Eula not being accepted.
+  scheduling_provider_->OnPersistentSchedulerWakeUp();
+  scheduling_provider_->OnNTPOpened();
+  scheduling_provider_->OnBrowserForegrounded();
+  scheduling_provider_->OnBrowserColdStart();
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ShouldFetchWhenEulaGetsAccepted) {
+  // Do not run the test if Eula notifier is not available.

[tschumann, 2017/03/22 17:56:41]

Only run this tests on platforms supporting Eula.

[jkrcal, 2017/03/23 09:29:42]

Done.

+  if (!IsEulaNotifierAvailable()) {
+    return;
+  }
+  // First enable the scheduler.
+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
+  ActivateUnderlyingProviderWithoutEula();
+
+  // Make one (ignored) call to make sure we are interested in eula state.
+  scheduling_provider_->OnPersistentSchedulerWakeUp();
+
+  // Accepting Eula afterwards results in a background fetch.
+  EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_));
+  SetEulaAcceptedPref();
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
        ShouldIgnoreSignalsWhenDisabledByParam) {
   // First set an empty list of allowed trigger types.
   SetVariationParameter("scheduler_trigger_types", "-");