Fully initialize the Offline Helper pref every session

This causes the offline pref value to be fully reset every session, some
time after startup. When the pref is actually updated depends on when
the offline page model schedules the task, which happens on a task
thread.

Bug: 914577
Change-Id: I6d4d4140f042908394bca8150481a78cd7bbb832
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1597118Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Commit-Queue: Robert Ogden <robertogden@chromium.org>
Cr-Commit-Position: refs/heads/master@{#657309}

chrome/browser/previews/previews_offline_helper.cc

@@ -8,6 +8,7 @@
 #include <string>
 #include <vector>
 
+#include "base/bind.h"
 #include "base/feature_list.h"
 #include "base/hash/hash.h"
 #include "base/optional.h"
@@ -92,13 +93,29 @@ void RemoveStaleOfflinePageEntries(base::DictionaryValue* dict) {
   }
 }
 
+void AddSingleOfflineItemEntry(
+    base::DictionaryValue* available_pages,
+    const offline_pages::OfflinePageItem& added_page) {
+  available_pages->SetKey(
+      HashURL(added_page.url),
+      base::Value(TimeToDictionaryValue(added_page.creation_time)));
+
+  // Also remember the original url (pre-redirects) if one exists.
+  if (!added_page.original_url_if_different.is_empty()) {
+    available_pages->SetKey(
+        HashURL(added_page.original_url_if_different),
+        base::Value(TimeToDictionaryValue(added_page.creation_time)));
+  }
+}
+
 }  // namespace
 
 PreviewsOfflineHelper::PreviewsOfflineHelper(
     content::BrowserContext* browser_context)
     : pref_service_(nullptr),
       available_pages_(std::make_unique<base::DictionaryValue>()),
-      offline_page_model_(nullptr) {
+      offline_page_model_(nullptr),
+      weak_factory_(this) {
   if (!browser_context || browser_context->IsOffTheRecord())
     return;
 
@@ -116,8 +133,12 @@ PreviewsOfflineHelper::PreviewsOfflineHelper(
   offline_page_model_ =
       offline_pages::OfflinePageModelFactory::GetForBrowserContext(
           browser_context);
-  if (offline_page_model_)
+  if (offline_page_model_) {
     offline_page_model_->AddObserver(this);
+    offline_page_model_->GetAllPages(
+        base::BindOnce(&PreviewsOfflineHelper::UpdateAllPrefEntries,
+                       weak_factory_.GetWeakPtr()));
+  }
 #endif  // BUILDFLAG(ENABLE_OFFLINE_PAGES)
 }
 
@@ -174,28 +195,29 @@ void PreviewsOfflineHelper::Shutdown() {
   }
 }
 
+void PreviewsOfflineHelper::UpdateAllPrefEntries(
+    const offline_pages::MultipleOfflinePageItemResult& pages) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  // Totally reset the pref with the given vector. We presume that the given
+  // |pages| are a full result from a Offline DB query which we take as the
+  // source of truth.
+  available_pages_->Clear();
+  for (const offline_pages::OfflinePageItem& page : pages)
+    AddSingleOfflineItemEntry(available_pages_.get(), page);
+  RemoveStaleOfflinePageEntries(available_pages_.get());
+  UpdatePref();
+}
+
 void PreviewsOfflineHelper::OfflinePageModelLoaded(
     offline_pages::OfflinePageModel* model) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  // TODO(robertogden): Implement.
+  // Ignored.
 }
 
 void PreviewsOfflineHelper::OfflinePageAdded(
     offline_pages::OfflinePageModel* model,
     const offline_pages::OfflinePageItem& added_page) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
-  available_pages_->SetKey(
-      HashURL(added_page.url),
-      base::Value(TimeToDictionaryValue(added_page.creation_time)));
-
-  // Also remember the original url (pre-redirects) if one exists.
-  if (!added_page.original_url_if_different.is_empty()) {
-    available_pages_->SetKey(
-        HashURL(added_page.original_url_if_different),
-        base::Value(TimeToDictionaryValue(added_page.creation_time)));
-  }
-
+  AddSingleOfflineItemEntry(available_pages_.get(), added_page);
   RemoveStaleOfflinePageEntries(available_pages_.get());
   UpdatePref();
 }

chrome/browser/previews/previews_offline_helper.h

@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/sequence_checker.h"
 #include "base/values.h"
 #include "components/offline_pages/core/offline_page_model.h"
@@ -40,6 +41,11 @@ class PreviewsOfflineHelper : public offline_pages::OfflinePageModel::Observer {
   // Removes |this| as an observer from offline pages.
   void Shutdown();
 
+  // Updates all entries in the pref with the given result from an offline page
+  // database query.
+  void UpdateAllPrefEntries(
+      const offline_pages::MultipleOfflinePageItemResult& pages);
+
   // offline_pages::OfflinePageModel::Observer:
   void OfflinePageModelLoaded(offline_pages::OfflinePageModel* model) override;
   void OfflinePageAdded(
@@ -66,6 +72,8 @@ class PreviewsOfflineHelper : public offline_pages::OfflinePageModel::Observer {
 
   SEQUENCE_CHECKER(sequence_checker_);
 
+  base::WeakPtrFactory<PreviewsOfflineHelper> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(PreviewsOfflineHelper);
 };
 

chrome/browser/previews/previews_offline_helper_unittest.cc

@@ -239,3 +239,28 @@ TEST_F(PreviewsOfflineHelperTest, TestMaxPrefSize) {
   EXPECT_TRUE(
       helper->ShouldAttemptOfflinePreview(GURL("http://test.second.com")));
 }
+
+TEST_F(PreviewsOfflineHelperTest, TestUpdateAllPrefEntries) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      previews::features::kOfflinePreviewsFalsePositivePrevention);
+
+  PreviewsOfflineHelper* helper = NewHelper();
+  base::Time now = base::Time::Now();
+  base::Time expired = now -
+                       previews::params::OfflinePreviewFreshnessDuration() -
+                       base::TimeDelta::FromHours(1);
+
+  helper->OfflinePageAdded(nullptr,
+                           MakeAddedPageItem("http://cleared.com", "", now));
+  EXPECT_TRUE(helper->ShouldAttemptOfflinePreview(GURL("http://cleared.com")));
+
+  helper->UpdateAllPrefEntries(
+      {MakeAddedPageItem("http://new.com", "", now),
+       MakeAddedPageItem("http://new2.com", "", now),
+       MakeAddedPageItem("http://expired.com", "", expired)});
+  EXPECT_FALSE(helper->ShouldAttemptOfflinePreview(GURL("http://cleared.com")));
+  EXPECT_TRUE(helper->ShouldAttemptOfflinePreview(GURL("http://new.com")));
+  EXPECT_TRUE(helper->ShouldAttemptOfflinePreview(GURL("http://new2.com")));
+  EXPECT_FALSE(helper->ShouldAttemptOfflinePreview(GURL("http://expired.com")));
+}