[Media Feeds] Post the task to fetch top feeds periodically.

Bug: 1064751
Change-Id: Id8bda00d202e1e5e64642a4cdacfbaebd26c50e6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2250929Reviewed-by: Becca Hughes <beccahughes@chromium.org>
Commit-Queue: Sam Bowen <sgbowen@google.com>
Cr-Commit-Position: refs/heads/master@{#780922}

chrome/browser/media/feeds/media_feeds_service.cc

@@ -9,6 +9,7 @@
 #include "base/feature_list.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/optional.h"
+#include "base/task/task_traits.h"
 #include "base/threading/thread_checker.h"
 #include "base/time/clock.h"
 #include "base/time/default_clock.h"
@@ -28,6 +29,8 @@
 #include "components/safe_search_api/safe_search/safe_search_url_checker_client.h"
 #include "components/safe_search_api/url_checker.h"
 #include "content/public/browser/browser_context.h"
+#include "content/public/browser/browser_task_traits.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/storage_partition.h"
 #include "media/base/media_switches.h"
 #include "mojo/public/cpp/bindings/receiver.h"
@@ -135,6 +138,13 @@ class CookieChangeListener : public network::mojom::CookieChangeListener {
 const char MediaFeedsService::kSafeSearchResultHistogramName[] =
     "Media.Feeds.SafeSearch.Result";
 
+// static
+constexpr base::TimeDelta MediaFeedsService::kTimeBetweenBackgroundFetches;
+
+// static
+constexpr base::TimeDelta
+    MediaFeedsService::kTimeBetweenNonCachedBackgroundFetches;
+
 // The maximum number of feeds to fetch when getting the top feeds.
 const int kMaxTopFeedsToFetch = 5;
 
@@ -157,6 +167,17 @@ MediaFeedsService::MediaFeedsService(Profile* profile)
       prefs::kMediaFeedsSafeSearchEnabled,
       base::BindRepeating(&MediaFeedsService::OnSafeSearchPrefChanged,
                           weak_factory_.GetWeakPtr()));
+  pref_change_registrar_.Add(
+      prefs::kMediaFeedsBackgroundFetching,
+      base::BindRepeating(&MediaFeedsService::OnBackgroundFetchingPrefChanged,
+                          weak_factory_.GetWeakPtr()));
+
+  if (IsBackgroundFetchingEnabled()) {
+    content::GetUIThreadTaskRunner({})->PostTask(
+        FROM_HERE,
+        base::BindOnce(&MediaFeedsService::FetchTopMediaFeeds,
+                       weak_factory_.GetWeakPtr(), base::OnceClosure()));
+  }
 }
 
 // static
@@ -367,7 +388,14 @@ void MediaFeedsService::OnGotTopFeeds(
     ++it;
   }
 
-  std::move(callback).Run();
+  content::GetUIThreadTaskRunner({})->PostDelayedTask(
+      FROM_HERE,
+      base::BindOnce(&MediaFeedsService::FetchTopMediaFeeds,
+                     weak_factory_.GetWeakPtr(), base::OnceClosure()),
+      kTimeBetweenBackgroundFetches);
+
+  if (callback)
+    std::move(callback).Run();
 }
 
 void MediaFeedsService::OnCheckURLDone(
@@ -544,6 +572,16 @@ void MediaFeedsService::OnSafeSearchPrefChanged() {
                      weak_factory_.GetWeakPtr()));
 }
 
+void MediaFeedsService::OnBackgroundFetchingPrefChanged() {
+  if (!IsBackgroundFetchingEnabled())
+    return;
+
+  content::GetUIThreadTaskRunner({})->PostTask(
+      FROM_HERE,
+      base::BindOnce(&MediaFeedsService::FetchTopMediaFeeds,
+                     weak_factory_.GetWeakPtr(), base::OnceClosure()));
+}
+
 void MediaFeedsService::OnResetOriginFromCookie(
     const url::Origin& origin,
     const bool include_subdomains,
@@ -563,13 +601,14 @@ MediaFeedsService::GetBackgroundFetchFeedSettings(
   settings.should_fetch = false;
   settings.bypass_cache = false;
 
-  // Fetches should be spaced 15 minutes apart with exponential backoff
-  // based on how many sequential times the fetch has failed.
+  // Fetches should be spaced with exponential backoff based on how many
+  // sequential times the fetch has failed.
   if (feed->last_fetch_time.has_value()) {
     // TODO(crbug.com/1064751): Consider using net::BackoffEntry for this.
     base::Time next_fetch_time =
         feed->last_fetch_time.value() +
-        base::TimeDelta::FromMinutes(15 * pow(2, feed->fetch_failed_count));
+        base::TimeDelta::FromMinutes(kTimeBetweenBackgroundFetches.InMinutes() *
+                                     pow(2, feed->fetch_failed_count));
     settings.should_fetch = next_fetch_time < clock_->Now();
   }
 
@@ -577,7 +616,7 @@ MediaFeedsService::GetBackgroundFetchFeedSettings(
   // fetch.
   if (feed->last_fetch_time_not_cache_hit.has_value()) {
     base::Time next_fetch_time = feed->last_fetch_time_not_cache_hit.value() +
-                                 base::TimeDelta::FromHours(24);
+                                 kTimeBetweenNonCachedBackgroundFetches;
     if (next_fetch_time < clock_->Now()) {
       settings.should_fetch = true;
       settings.bypass_cache = true;

chrome/browser/media/feeds/media_feeds_service.h

@@ -11,6 +11,7 @@
 #include "base/callback_forward.h"
 #include "base/memory/weak_ptr.h"
 #include "base/threading/thread_checker.h"
+#include "base/time/time.h"
 #include "chrome/browser/media/feeds/media_feeds_converter.h"
 #include "chrome/browser/media/feeds/media_feeds_fetcher.h"
 #include "chrome/browser/media/feeds/media_feeds_store.mojom.h"
@@ -44,6 +45,16 @@ class MediaFeedsService : public KeyedService {
  public:
   static const char kSafeSearchResultHistogramName[];
 
+  // Time to wait between background fetch delayed tasks.
+  static constexpr base::TimeDelta kTimeBetweenBackgroundFetches =
+      base::TimeDelta::FromMinutes(15);
+
+  // If this much time has passed since the last time we got a non-cached, fresh
+  // version of the feed, we should bypass the cache on the next background
+  // fetch of the feed.
+  static constexpr base::TimeDelta kTimeBetweenNonCachedBackgroundFetches =
+      base::TimeDelta::FromHours(24);
+
   using FetchMediaFeedCallback =
       base::OnceCallback<void(const std::string& logs)>;
 
@@ -148,6 +159,8 @@ class MediaFeedsService : public KeyedService {
 
   void OnSafeSearchPrefChanged();
 
+  void OnBackgroundFetchingPrefChanged();
+
   void OnResetOriginFromCookie(const url::Origin& origin,
                                const bool include_subdomains,
                                const std::string& name,

chrome/browser/media/feeds/media_feeds_service_unittest.cc

@@ -165,7 +165,9 @@ const char kFirstItemPlayNextActionURL[] = "https://www.example.com/next";
 
 class MediaFeedsServiceTest : public ChromeRenderViewHostTestHarness {
  public:
-  MediaFeedsServiceTest() = default;
+  MediaFeedsServiceTest()
+      : ChromeRenderViewHostTestHarness(
+            base::test::TaskEnvironment::TimeSource::MOCK_TIME) {}
 
   void SetUp() override {
     features_.InitWithFeatures(
@@ -189,8 +191,9 @@ class MediaFeedsServiceTest : public ChromeRenderViewHostTestHarness {
             &url_loader_factory_);
   }
 
-  void AdvanceClock(base::TimeDelta time_delta) {
-    test_clock_.SetNow(base::Time::Now() + time_delta);
+  void AdvanceTime(base::TimeDelta time_delta) {
+    test_clock_.SetNow(test_clock_.Now() + time_delta);
+    task_environment()->FastForwardBy(time_delta);
   }
 
   base::Time Now() { return test_clock_.Now(); }
@@ -429,7 +432,7 @@ class MediaFeedsServiceTest : public ChromeRenderViewHostTestHarness {
         cookie_line.push_back("Domain=" + url.host());
 
       if (expired)
-        cookie_line.push_back("Expires=Wed, 21 Oct 2015 07:28:00 GMT");
+        cookie_line.push_back("Expires=Wed, 31 Dec 1969 07:28:00 GMT");
 
       std::unique_ptr<net::CanonicalCookie> cookie =
           net::CanonicalCookie::Create(url, base::JoinString(cookie_line, ";"),
@@ -2086,21 +2089,16 @@ TEST_F(MediaFeedsServiceTest, FetchTopMediaFeeds_SuccessNewFetch) {
 
   const GURL feed_url("https://www.google.com/feed");
 
-  SetBackgroundFetchingEnabled(true);
-
   // Store a Media Feed.
   GetMediaFeedsService()->DiscoverMediaFeed(feed_url);
   WaitForDB();
 
+  SetBackgroundFetchingEnabled(true);
+  task_environment()->RunUntilIdle();
+
   // FetchTopMediaFeeds should ignore the feed, as the origin does not have
   // enough watchtime.
-  {
-    base::RunLoop top_feeds_loop;
-    GetMediaFeedsService()->FetchTopMediaFeeds(top_feeds_loop.QuitClosure());
-    WaitForDB();
-    top_feeds_loop.Run();
-    ASSERT_FALSE(RespondToPendingFeedFetch(feed_url));
-  }
+  ASSERT_FALSE(RespondToPendingFeedFetch(feed_url));
 
   // Set the watchtime higher than the minimum threshold for top feeds.
   auto watchtime = base::TimeDelta::FromMinutes(45);
@@ -2109,14 +2107,10 @@ TEST_F(MediaFeedsServiceTest, FetchTopMediaFeeds_SuccessNewFetch) {
   GetMediaHistoryService()->SavePlayback(watch_time);
   WaitForDB();
 
-  // Now that there is high watchtime, the fetch should occur.
-  {
-    base::RunLoop top_feeds_loop;
-    GetMediaFeedsService()->FetchTopMediaFeeds(top_feeds_loop.QuitClosure());
-    WaitForDB();
-    top_feeds_loop.Run();
-    ASSERT_TRUE(RespondToPendingFeedFetch(feed_url));
-  }
+  // Now that there is high watchtime, the fetch should occur the next time
+  // background fetching happens.
+  AdvanceTime(MediaFeedsService::kTimeBetweenBackgroundFetches);
+  ASSERT_TRUE(RespondToPendingFeedFetch(feed_url));
 
   auto feeds = GetMediaFeedsSync();
 
@@ -2135,8 +2129,6 @@ TEST_F(MediaFeedsServiceTest, FetchTopMediaFeeds_SuccessFromCache) {
 
   const GURL feed_url("https://www.google.com/feed");
 
-  SetBackgroundFetchingEnabled(true);
-
   // Store a Media Feed.
   GetMediaFeedsService()->DiscoverMediaFeed(feed_url);
   WaitForDB();
@@ -2157,18 +2149,14 @@ TEST_F(MediaFeedsServiceTest, FetchTopMediaFeeds_SuccessFromCache) {
   ASSERT_TRUE(RespondToPendingFeedFetch(feed_url));
   run_loop.Run();
 
-  // After some time, fetch top feeds should refresh the feed.
-  AdvanceClock(base::TimeDelta::FromHours(1));
-
-  base::RunLoop top_feeds_loop;
-  GetMediaFeedsService()->FetchTopMediaFeeds(top_feeds_loop.QuitClosure());
-  WaitForDB();
+  // After some time, background fetching should refresh the feed from the
+  // cached initial fetch.
+  AdvanceTime(MediaFeedsService::kTimeBetweenBackgroundFetches);
+  SetBackgroundFetchingEnabled(true);
+  task_environment()->RunUntilIdle();
 
-  // It has been < 24 hrs, so it should fetch from the cached initial fetch.
   EXPECT_FALSE(GetCurrentRequestHasBypassCacheFlag());
-
   ASSERT_TRUE(RespondToPendingFeedFetch(feed_url));
-  top_feeds_loop.Run();
 
   auto feeds = GetMediaFeedsSync();
 
@@ -2189,8 +2177,6 @@ TEST_F(MediaFeedsServiceTest, FetchTopMediaFeeds_BacksOffFailedFetches) {
 
   const GURL feed_url("https://www.google.com/feed");
 
-  SetBackgroundFetchingEnabled(true);
-
   // Store a Media Feed.
   GetMediaFeedsService()->DiscoverMediaFeed(feed_url);
   WaitForDB();
@@ -2214,39 +2200,27 @@ TEST_F(MediaFeedsServiceTest, FetchTopMediaFeeds_BacksOffFailedFetches) {
     run_loop.Run();
   }
 
-  // No fetch should happen because of the backoff from failures.
-  {
-    AdvanceClock(base::TimeDelta::FromHours(1));
-
-    base::RunLoop top_feeds_loop;
-    GetMediaFeedsService()->FetchTopMediaFeeds(top_feeds_loop.QuitClosure());
-    WaitForDB();
+  AdvanceTime(base::TimeDelta::FromHours(1));
+  SetBackgroundFetchingEnabled(true);
+  task_environment()->RunUntilIdle();
 
-    ASSERT_FALSE(RespondToPendingFeedFetch(feed_url));
-    top_feeds_loop.Run();
-  }
+  // No fetch should happen because of the backoff from failures.
+  ASSERT_FALSE(RespondToPendingFeedFetch(feed_url));
 
   // After 24 hours, the feed should be fetched regardless of failure count.
-  {
-    AdvanceClock(base::TimeDelta::FromHours(24));
-
-    base::RunLoop top_feeds_loop;
-    GetMediaFeedsService()->FetchTopMediaFeeds(top_feeds_loop.QuitClosure());
-    WaitForDB();
+  AdvanceTime(MediaFeedsService::kTimeBetweenNonCachedBackgroundFetches);
+  task_environment()->RunUntilIdle();
 
-    // If we bypass failure count, we should also bypass cache.
-    EXPECT_TRUE(GetCurrentRequestHasBypassCacheFlag());
-
-    ASSERT_TRUE(RespondToPendingFeedFetch(feed_url));
-    top_feeds_loop.Run();
+  // If we bypass failure count, we should also bypass cache.
+  EXPECT_TRUE(GetCurrentRequestHasBypassCacheFlag());
+  ASSERT_TRUE(RespondToPendingFeedFetch(feed_url));
 
-    auto feeds = GetMediaFeedsSync();
+  auto feeds = GetMediaFeedsSync();
 
-    EXPECT_EQ(1u, feeds.size());
-    EXPECT_TRUE(feeds[0]->last_fetch_time_not_cache_hit);
-    EXPECT_EQ(media_feeds::mojom::FetchResult::kSuccess,
-              feeds[0]->last_fetch_result);
-  }
+  EXPECT_EQ(1u, feeds.size());
+  EXPECT_TRUE(feeds[0]->last_fetch_time_not_cache_hit);
+  EXPECT_EQ(media_feeds::mojom::FetchResult::kSuccess,
+            feeds[0]->last_fetch_result);
 
   histogram_tester.ExpectUniqueSample(
       MediaFeedsFetcher::kFetchSizeKbHistogramName, 15, 1);
@@ -2259,8 +2233,6 @@ TEST_F(MediaFeedsServiceTest, FetchTopMediaFeeds_SuccessBypassCache) {
 
   const GURL feed_url("https://www.google.com/feed");
 
-  SetBackgroundFetchingEnabled(true);
-
   // Store a Media Feed.
   GetMediaFeedsService()->DiscoverMediaFeed(feed_url);
   WaitForDB();
@@ -2281,17 +2253,13 @@ TEST_F(MediaFeedsServiceTest, FetchTopMediaFeeds_SuccessBypassCache) {
   ASSERT_TRUE(RespondToPendingFeedFetch(feed_url));
   run_loop.Run();
 
-  AdvanceClock(base::TimeDelta::FromHours(24));
-
-  base::RunLoop top_feeds_loop;
-  GetMediaFeedsService()->FetchTopMediaFeeds(top_feeds_loop.QuitClosure());
-  WaitForDB();
+  AdvanceTime(base::TimeDelta::FromHours(24));
+  SetBackgroundFetchingEnabled(true);
+  task_environment()->RunUntilIdle();
 
   // After a long time between fetches, we should bypass the cache.
   EXPECT_TRUE(GetCurrentRequestHasBypassCacheFlag());
-
   ASSERT_TRUE(RespondToPendingFeedFetch(feed_url));
-  top_feeds_loop.Run();
 
   auto feeds = GetMediaFeedsSync();
 
@@ -2310,8 +2278,6 @@ TEST_F(MediaFeedsServiceTest, FetchTopMediaFeeds_SuccessResetFeed) {
 
   const GURL feed_url("https://www.google.com/feed");
 
-  SetBackgroundFetchingEnabled(true);
-
   // Store a Media Feed.
   GetMediaFeedsService()->DiscoverMediaFeed(feed_url);
   WaitForDB();
@@ -2335,11 +2301,52 @@ TEST_F(MediaFeedsServiceTest, FetchTopMediaFeeds_SuccessResetFeed) {
                                          mojom::ResetReason::kVisit);
   WaitForDB();
 
-  base::RunLoop top_feeds_loop;
-  GetMediaFeedsService()->FetchTopMediaFeeds(top_feeds_loop.QuitClosure());
+  SetBackgroundFetchingEnabled(true);
+  task_environment()->RunUntilIdle();
+  ASSERT_TRUE(RespondToPendingFeedFetch(feed_url));
+
+  auto feeds = GetMediaFeedsSync();
+
+  EXPECT_EQ(1u, feeds.size());
+  EXPECT_TRUE(feeds[0]->last_fetch_time_not_cache_hit);
+  EXPECT_EQ(media_feeds::mojom::FetchResult::kSuccess,
+            feeds[0]->last_fetch_result);
+
+  histogram_tester.ExpectUniqueSample(
+      MediaFeedsFetcher::kFetchSizeKbHistogramName, 15, 2);
+}
+
+// After enabling the pref, top feeds should fetch immediately and then again
+// after 15 minutes.
+TEST_F(MediaFeedsServiceTest, FetchTopMediaFeeds_SuccessRepeatsPeriodically) {
+  base::HistogramTester histogram_tester;
+
+  const GURL feed_url("https://www.google.com/feed");
+
+  // Store a Media Feed.
+  GetMediaFeedsService()->DiscoverMediaFeed(feed_url);
+  WaitForDB();
+
+  // Set the watchtime higher than the minimum threshold for top feeds.
+  auto watchtime = base::TimeDelta::FromMinutes(45);
+  content::MediaPlayerWatchTime watch_time(
+      feed_url, feed_url.GetOrigin(), watchtime, base::TimeDelta(), true, true);
+  GetMediaHistoryService()->SavePlayback(watch_time);
   WaitForDB();
+
+  // Once we set this, background fetching should start automatically.
+  SetBackgroundFetchingEnabled(true);
+  task_environment()->RunUntilIdle();
+
+  // There should be only one fetch ready.
+  ASSERT_TRUE(RespondToPendingFeedFetch(feed_url));
+  ASSERT_FALSE(RespondToPendingFeedFetch(feed_url));
+
+  // Wait 15 minutes and the next fetch should be queued up.
+  AdvanceTime(MediaFeedsService::kTimeBetweenBackgroundFetches);
+  task_environment()->RunUntilIdle();
+
   ASSERT_TRUE(RespondToPendingFeedFetch(feed_url));
-  top_feeds_loop.Run();
 
   auto feeds = GetMediaFeedsSync();