Make NTP PrefetchedPageTracker to load Offline Pages database on demand.

The PrefetchedPagesTracker is used to see if suggestions have corresponding
offline pages, in which case they may be given longer lifetime before expiration.
The check is done after remote provider does a "Fetch", which brings new suggestions
and starts suggestions integration. The whole thing is asynchronous and can trigger
load of offline pages database right after the fetch rather than at the moment of
creation, as it does it currently.

Loading Offline Pages database on Chrome startup becomes too expensive as number
of offline pages grows so it needs to become more on demand.

TBR: vitaliii@chromium.org
Bug: 793109
Change-Id: I919c5c777e8363c194c49cce1fd86076b87ead4d
Reviewed-on: https://chromium-review.googlesource.com/906103Reviewed-by: Filip Gorski <fgorski@chromium.org>
Reviewed-by: Patrick Noland <pnoland@google.com>
Commit-Queue: Dmitry Titov <dimich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537476}

components/ntp_snippets/remote/prefetched_pages_tracker.h

@@ -21,11 +21,10 @@ class PrefetchedPagesTracker {
   // Whether the tracker has finished initialization.
   virtual bool IsInitialized() const = 0;
 
-  // Add a callback, which will be called when the initialization is completed.
-  // If the tracker has been initialized already, the callback is called
-  // immediately.
-  virtual void AddInitializationCompletedCallback(
-      base::OnceCallback<void()> callback) = 0;
+  // Starts asynchronous initialization. Callback will be called when the
+  // initialization is completed. If the tracker has been initialized already,
+  // the callback is called immediately.
+  virtual void Initialize(base::OnceCallback<void()> callback) = 0;
 
   virtual bool PrefetchedOfflinePageExists(const GURL& url) const = 0;
 };

components/ntp_snippets/remote/prefetched_pages_tracker_impl.cc

@@ -33,12 +33,6 @@ PrefetchedPagesTrackerImpl::PrefetchedPagesTrackerImpl(
       offline_page_model_(offline_page_model),
       weak_ptr_factory_(this) {
   DCHECK(offline_page_model_);
-  // If Offline Page model is not loaded yet, it will process our query
-  // once it has finished loading.
-  offline_page_model_->GetPagesByNamespace(
-      offline_pages::kSuggestedArticlesNamespace,
-      base::Bind(&PrefetchedPagesTrackerImpl::Initialize,
-                 weak_ptr_factory_.GetWeakPtr()));
 }
 
 PrefetchedPagesTrackerImpl::~PrefetchedPagesTrackerImpl() {
@@ -49,12 +43,22 @@ bool PrefetchedPagesTrackerImpl::IsInitialized() const {
   return initialized_;
 }
 
-void PrefetchedPagesTrackerImpl::AddInitializationCompletedCallback(
+void PrefetchedPagesTrackerImpl::Initialize(
     base::OnceCallback<void()> callback) {
   if (IsInitialized()) {
     std::move(callback).Run();
+  } else {
+    initialization_completed_callbacks_.push_back(std::move(callback));
+    // The call to get pages might be already in flight, started by previous
+    // calls to this method. In this case, there is at least one callback
+    // already waiting.
+    if (initialization_completed_callbacks_.size() == 1) {
+      offline_page_model_->GetPagesByNamespace(
+          offline_pages::kSuggestedArticlesNamespace,
+          base::BindRepeating(&PrefetchedPagesTrackerImpl::OfflinePagesLoaded,
+                              weak_ptr_factory_.GetWeakPtr()));
+    }
   }
-  initialization_completed_callbacks_.push_back(std::move(callback));
 }
 
 bool PrefetchedPagesTrackerImpl::PrefetchedOfflinePageExists(
@@ -100,7 +104,7 @@ void PrefetchedPagesTrackerImpl::OfflinePageDeleted(
   offline_id_to_url_mapping_.erase(offline_id_it);
 }
 
-void PrefetchedPagesTrackerImpl::Initialize(
+void PrefetchedPagesTrackerImpl::OfflinePagesLoaded(
     const std::vector<OfflinePageItem>& all_prefetched_offline_pages) {
   for (const OfflinePageItem& item : all_prefetched_offline_pages) {
     DCHECK(IsOfflineItemPrefetchedPage(item));
@@ -112,6 +116,7 @@ void PrefetchedPagesTrackerImpl::Initialize(
   for (auto& callback : initialization_completed_callbacks_) {
     std::move(callback).Run();
   }
+  initialization_completed_callbacks_.clear();
 }
 
 void PrefetchedPagesTrackerImpl::AddOfflinePage(

components/ntp_snippets/remote/prefetched_pages_tracker_impl.h

@@ -32,8 +32,7 @@ class PrefetchedPagesTrackerImpl
 
   // PrefetchedPagesTracker implementation
   bool IsInitialized() const override;
-  void AddInitializationCompletedCallback(
-      base::OnceCallback<void()> callback) override;
+  void Initialize(base::OnceCallback<void()> callback) override;
   bool PrefetchedOfflinePageExists(const GURL& url) const override;
 
   // OfflinePageModel::Observer implementation.
@@ -46,8 +45,8 @@ class PrefetchedPagesTrackerImpl
       override;
 
  private:
-  void Initialize(const std::vector<offline_pages::OfflinePageItem>&
-                      all_prefetched_offline_pages);
+  void OfflinePagesLoaded(const std::vector<offline_pages::OfflinePageItem>&
+                              all_prefetched_offline_pages);
   void AddOfflinePage(const offline_pages::OfflinePageItem& offline_page_item);
 
   bool initialized_;

components/ntp_snippets/remote/prefetched_pages_tracker_impl_unittest.cc

@@ -69,11 +69,12 @@ class PrefetchedPagesTrackerImplTest : public ::testing::Test {
 };
 
 TEST_F(PrefetchedPagesTrackerImplTest,
-       ShouldRetrievePrefetchedEarlierSuggestionsOnStartup) {
+       ShouldRetrievePrefetchedEarlierSuggestionsOnInitialize) {
   (*fake_offline_page_model()->mutable_items()) = {
       CreateOfflinePageItem(GURL("http://prefetched.com"),
                             offline_pages::kSuggestedArticlesNamespace)};
   PrefetchedPagesTrackerImpl tracker(fake_offline_page_model());
+  tracker.Initialize(base::BindOnce([] {}));
 
   ASSERT_FALSE(
       tracker.PrefetchedOfflinePageExists(GURL("http://not_added_url.com")));
@@ -85,6 +86,7 @@ TEST_F(PrefetchedPagesTrackerImplTest,
        ShouldAddNewPrefetchedPagesWhenNotified) {
   fake_offline_page_model()->mutable_items()->clear();
   PrefetchedPagesTrackerImpl tracker(fake_offline_page_model());
+  tracker.Initialize(base::BindOnce([] {}));
 
   ASSERT_FALSE(
       tracker.PrefetchedOfflinePageExists(GURL("http://prefetched.com")));
@@ -100,6 +102,7 @@ TEST_F(PrefetchedPagesTrackerImplTest,
        ShouldIgnoreOtherTypesOfOfflinePagesWhenNotified) {
   fake_offline_page_model()->mutable_items()->clear();
   PrefetchedPagesTrackerImpl tracker(fake_offline_page_model());
+  tracker.Initialize(base::BindOnce([] {}));
 
   ASSERT_FALSE(tracker.PrefetchedOfflinePageExists(
       GURL("http://manually_downloaded.com")));
@@ -117,6 +120,7 @@ TEST_F(PrefetchedPagesTrackerImplTest,
       CreateOfflinePageItem(GURL("http://manually_downloaded.com"),
                             offline_pages::kNTPSuggestionsNamespace)};
   PrefetchedPagesTrackerImpl tracker(fake_offline_page_model());
+  tracker.Initialize(base::BindOnce([] {}));
 
   ASSERT_FALSE(tracker.PrefetchedOfflinePageExists(
       GURL("http://manually_downloaded.com")));
@@ -130,6 +134,7 @@ TEST_F(PrefetchedPagesTrackerImplTest, ShouldDeletePrefetchedURLWhenNotified) {
                             offline_pages::kSuggestedArticlesNamespace);
   (*fake_offline_page_model()->mutable_items()) = {item};
   PrefetchedPagesTrackerImpl tracker(fake_offline_page_model());
+  tracker.Initialize(base::BindOnce([] {}));
 
   ASSERT_TRUE(
       tracker.PrefetchedOfflinePageExists(GURL("http://prefetched.com")));
@@ -151,6 +156,7 @@ TEST_F(PrefetchedPagesTrackerImplTest,
   (*fake_offline_page_model()->mutable_items()) = {prefetched_item,
                                                    manually_downloaded_item};
   PrefetchedPagesTrackerImpl tracker(fake_offline_page_model());
+  tracker.Initialize(base::BindOnce([] {}));
 
   ASSERT_TRUE(
       tracker.PrefetchedOfflinePageExists(GURL("http://prefetched.com")));
@@ -163,11 +169,12 @@ TEST_F(PrefetchedPagesTrackerImplTest,
 }
 
 TEST_F(PrefetchedPagesTrackerImplTest,
-       ShouldReportAsNotInitializedBeforeInitialization) {
+       ShouldReportAsNotInitializedBeforeReceivedArticles) {
   EXPECT_CALL(
       *mock_offline_page_model(),
       GetPagesByNamespace(offline_pages::kSuggestedArticlesNamespace, _));
   PrefetchedPagesTrackerImpl tracker(mock_offline_page_model());
+  tracker.Initialize(base::BindOnce([] {}));
   EXPECT_FALSE(tracker.IsInitialized());
 }
 
@@ -179,6 +186,7 @@ TEST_F(PrefetchedPagesTrackerImplTest,
       GetPagesByNamespace(offline_pages::kSuggestedArticlesNamespace, _))
       .WillOnce(SaveArg<1>(&offline_pages_callback));
   PrefetchedPagesTrackerImpl tracker(mock_offline_page_model());
+  tracker.Initialize(base::BindOnce([] {}));
 
   ASSERT_FALSE(tracker.IsInitialized());
   offline_pages_callback.Run(std::vector<OfflinePageItem>());
@@ -195,8 +203,7 @@ TEST_F(PrefetchedPagesTrackerImplTest, ShouldCallCallbackAfterInitialization) {
 
   base::MockCallback<base::OnceCallback<void()>>
       mock_initialization_completed_callback;
-  tracker.AddInitializationCompletedCallback(
-      mock_initialization_completed_callback.Get());
+  tracker.Initialize(mock_initialization_completed_callback.Get());
   EXPECT_CALL(mock_initialization_completed_callback, Run());
   offline_pages_callback.Run(std::vector<OfflinePageItem>());
 }
@@ -213,10 +220,8 @@ TEST_F(PrefetchedPagesTrackerImplTest,
   base::MockCallback<base::OnceCallback<void()>>
       first_mock_initialization_completed_callback,
       second_mock_initialization_completed_callback;
-  tracker.AddInitializationCompletedCallback(
-      first_mock_initialization_completed_callback.Get());
-  tracker.AddInitializationCompletedCallback(
-      second_mock_initialization_completed_callback.Get());
+  tracker.Initialize(first_mock_initialization_completed_callback.Get());
+  tracker.Initialize(second_mock_initialization_completed_callback.Get());
   EXPECT_CALL(first_mock_initialization_completed_callback, Run());
   EXPECT_CALL(second_mock_initialization_completed_callback, Run());
   offline_pages_callback.Run(std::vector<OfflinePageItem>());
@@ -230,13 +235,14 @@ TEST_F(PrefetchedPagesTrackerImplTest,
       GetPagesByNamespace(offline_pages::kSuggestedArticlesNamespace, _))
       .WillOnce(SaveArg<1>(&offline_pages_callback));
   PrefetchedPagesTrackerImpl tracker(mock_offline_page_model());
+  tracker.Initialize(base::BindOnce([] {}));
+
   offline_pages_callback.Run(std::vector<OfflinePageItem>());
 
   base::MockCallback<base::OnceCallback<void()>>
       mock_initialization_completed_callback;
   EXPECT_CALL(mock_initialization_completed_callback, Run());
-  tracker.AddInitializationCompletedCallback(
-      mock_initialization_completed_callback.Get());
+  tracker.Initialize(mock_initialization_completed_callback.Get());
 }
 
 TEST_F(PrefetchedPagesTrackerImplTest,
@@ -249,6 +255,7 @@ TEST_F(PrefetchedPagesTrackerImplTest,
                             offline_pages::kSuggestedArticlesNamespace);
   (*fake_offline_page_model()->mutable_items()) = {first_item, second_item};
   PrefetchedPagesTrackerImpl tracker(fake_offline_page_model());
+  tracker.Initialize(base::BindOnce([] {}));
 
   ASSERT_TRUE(
       tracker.PrefetchedOfflinePageExists(GURL("http://prefetched.com")));
@@ -273,6 +280,7 @@ TEST_F(PrefetchedPagesTrackerImplTest,
                             offline_pages::kSuggestedArticlesNamespace);
   (*fake_offline_page_model()->mutable_items()) = {first_item, second_item};
   PrefetchedPagesTrackerImpl tracker(fake_offline_page_model());
+  tracker.Initialize(base::BindOnce([] {}));
 
   ASSERT_TRUE(
       tracker.PrefetchedOfflinePageExists(GURL("http://prefetched.com")));

components/ntp_snippets/remote/remote_suggestions_provider_impl.cc

@@ -955,11 +955,10 @@ void RemoteSuggestionsProviderImpl::OnFetchFinished(
   if (IsKeepingPrefetchedSuggestionsEnabled() && prefetched_pages_tracker_ &&
       !prefetched_pages_tracker_->IsInitialized()) {
     // Wait until the tracker is initialized.
-    prefetched_pages_tracker_->AddInitializationCompletedCallback(
-        base::BindOnce(&RemoteSuggestionsProviderImpl::OnFetchFinished,
-                       base::Unretained(this), std::move(callback),
-                       interactive_request, status,
-                       std::move(fetched_categories)));
+    prefetched_pages_tracker_->Initialize(base::BindOnce(
+        &RemoteSuggestionsProviderImpl::OnFetchFinished, base::Unretained(this),
+        std::move(callback), interactive_request, status,
+        std::move(fetched_categories)));
     return;
   }
 

components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc

@@ -268,13 +268,10 @@ class MockPrefetchedPagesTracker : public PrefetchedPagesTracker {
 
   // GMock does not support movable-only types (e.g. OnceCallback), therefore,
   // the call is redirected to a mock method with a pointer to the callback.
-  void AddInitializationCompletedCallback(
-      base::OnceCallback<void()> callback) override {
-    AddInitializationCompletedCallback(&callback);
+  void Initialize(base::OnceCallback<void()> callback) override {
+    Initialize(&callback);
   }
-  MOCK_METHOD1(AddInitializationCompletedCallback,
-               void(base::OnceCallback<void()>* callback));
-
+  MOCK_METHOD1(Initialize, void(base::OnceCallback<void()>* callback));
   MOCK_CONST_METHOD1(PrefetchedOfflinePageExists, bool(const GURL& url));
 };
 
@@ -3283,7 +3280,7 @@ TEST_F(RemoteSuggestionsProviderImplTest,
 
   base::OnceCallback<void()> initialization_completed_callback;
   EXPECT_CALL(*mock_tracker, IsInitialized()).WillRepeatedly(Return(false));
-  EXPECT_CALL(*mock_tracker, AddInitializationCompletedCallback(_))
+  EXPECT_CALL(*mock_tracker, Initialize(_))
       .WillOnce(MoveFirstArgumentPointeeTo(&initialization_completed_callback));
   std::vector<FetchedCategory> fetched_categories;
   fetched_categories.push_back(