[Feed] Wire up FeedOfflineHost with PrefetchService

Bug: 866123
Change-Id: I89153c2871010027fc39a47b990f70d91b400f04
Reviewed-on: https://chromium-review.googlesource.com/c/1272658
Commit-Queue: Sky Malice <skym@chromium.org>
Reviewed-by: Filip Gorski <fgorski@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598685}

chrome/android/javatests/src/org/chromium/chrome/browser/feed/FeedRefreshTaskTest.java

@@ -13,8 +13,10 @@ import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 
+import org.chromium.base.CommandLine;
 import org.chromium.base.ThreadUtils;
 import org.chromium.base.test.util.CommandLineFlags;
+import org.chromium.chrome.browser.ChromeFeatureList;
 import org.chromium.chrome.browser.ChromeSwitches;
 import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
 import org.chromium.chrome.test.ChromeTabbedActivityTestRule;
@@ -30,7 +32,8 @@ import java.util.List;
  * Unit tests for {@link FeedSchedulerBridge}.
  */
 @RunWith(ChromeJUnit4ClassRunner.class)
-@CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE})
+@CommandLineFlags.Add(ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE)
+//@Features.EnableFeatures(ChromeFeatureList.INTEREST_FEED_CONTENT_SUGGESTIONS)
 public class FeedRefreshTaskTest {
     @Rule
     public ChromeTabbedActivityTestRule mActivityTestRule = new ChromeTabbedActivityTestRule();
@@ -69,6 +72,13 @@ public class FeedRefreshTaskTest {
 
     @Before
     public void setUp() throws InterruptedException {
+        // TODO(https://crbug.com/894334): Replace with @Features.EnableFeatures when presubmit
+        // forced format is fixed.
+        CommandLine commandLine = CommandLine.getInstance();
+        String oldValue = commandLine.getSwitchValue("enable-features");
+        commandLine.appendSwitchWithValue("enable-features",
+                oldValue + "," + ChromeFeatureList.INTEREST_FEED_CONTENT_SUGGESTIONS);
+
         mTaskScheduler = new TestBackgroundTaskScheduler();
         BackgroundTaskSchedulerFactory.setSchedulerForTesting(mTaskScheduler);
 

chrome/browser/android/feed/feed_offline_bridge.cc

@@ -57,6 +57,9 @@ FeedOfflineBridge::FeedOfflineBridge(const JavaRef<jobject>& j_this,
       offline_host_(offline_host),
       weak_factory_(this) {
   DCHECK(offline_host_);
+  // The host guarantees to not invoke these callbacks until Initialize() exits.
+  // This is important because until the Java bridge's constructor finishes, any
+  // attempt to cross JNI from native to Java will fail.
   offline_host_->Initialize(
       base::BindRepeating(&FeedOfflineBridge::TriggerGetKnownContent,
                           weak_factory_.GetWeakPtr()),
@@ -122,14 +125,26 @@ void FeedOfflineBridge::AppendContentMetadata(
     const base::android::JavaRef<jstring>& j_favicon_url,
     const base::android::JavaRef<jstring>& j_snippet) {
   ContentMetadata metadata;
+  DCHECK(!j_url.is_null());
   metadata.url = base::android::ConvertJavaStringToUTF8(env, j_url);
+  DCHECK(!j_title.is_null());
   metadata.title = base::android::ConvertJavaStringToUTF8(env, j_title);
   metadata.time_published = base::Time::FromJavaTime(j_time_published_ms);
-  metadata.image_url = base::android::ConvertJavaStringToUTF8(env, j_image_url);
-  metadata.publisher = base::android::ConvertJavaStringToUTF8(env, j_publisher);
+  if (!j_image_url.is_null()) {
+    metadata.image_url =
+        base::android::ConvertJavaStringToUTF8(env, j_image_url);
+  }
+  if (!j_publisher.is_null()) {
+    metadata.publisher =
+        base::android::ConvertJavaStringToUTF8(env, j_publisher);
+  }
+  if (!j_favicon_url.is_null()) {
     metadata.favicon_url =
         base::android::ConvertJavaStringToUTF8(env, j_favicon_url);
+  }
+  if (!j_snippet.is_null()) {
     metadata.snippet = base::android::ConvertJavaStringToUTF8(env, j_snippet);
+  }
   known_content_metadata_buffer_.push_back(std::move(metadata));
 }
 

components/feed/content/feed_offline_host.cc

@@ -9,6 +9,7 @@
 #include "base/bind.h"
 #include "base/hash.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "components/feed/core/feed_scheduler_host.h"
 #include "components/offline_pages/core/prefetch/prefetch_service.h"
 #include "url/gurl.h"
@@ -139,7 +140,18 @@ void FeedOfflineHost::Initialize(
   trigger_get_known_content_ = trigger_get_known_content;
   notify_status_change_ = notify_status_change;
   offline_page_model_->AddObserver(this);
-  // TODO(skym): Post task to call PrefetchService::SetSuggestionProvider().
+  // The host guarantees that the two callbacks passed into this method will not
+  // be invoked until Initialize() has exited. To guarantee this, the host
+  // cannot call SetSuggestionProvider() in task, because that would give
+  // Prefetch the ability to run |trigger_get_known_content_| immediately.
+  // PostTask is used to delay when SetSuggestionProvider() is called.
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::BindOnce(&FeedOfflineHost::SetSuggestionProvider,
+                                weak_factory_.GetWeakPtr()));
+}
+
+void FeedOfflineHost::SetSuggestionProvider() {
+  prefetch_service_->SetSuggestionProvider(this);
 }
 
 base::Optional<int64_t> FeedOfflineHost::GetOfflineId(const std::string& url) {
@@ -166,11 +178,13 @@ void FeedOfflineHost::GetOfflineStatus(
 }
 
 void FeedOfflineHost::OnContentRemoved(std::vector<std::string> urls) {
-  // TODO(skym): Call PrefetchService::RemoveSuggestion().
+  for (const std::string& url : urls) {
+    prefetch_service_->RemoveSuggestion(GURL(url));
+  }
 }
 
 void FeedOfflineHost::OnNewContentReceived() {
-  // TODO(skym): Call PrefetchService::NewSuggestionsAvailable().
+  prefetch_service_->NewSuggestionsAvailable();
 }
 
 void FeedOfflineHost::OnNoListeners() {

components/feed/content/feed_offline_host.h

@@ -47,9 +47,15 @@ class FeedOfflineHost : public offline_pages::SuggestionsProvider,
   // Initialize with callbacks to call into bridge/Java side. Should only be
   // called once, and done as soon as the bridge is ready. The FeedOfflineHost
   // will not be fully ready to perform its function without these dependencies.
+  // Neither of these callbacks will be invoked until after this method exits.
   void Initialize(const base::RepeatingClosure& trigger_get_known_content,
                   const NotifyStatusChangeCallback& notify_status_change);
 
+  // Called during initialization make ourselves known to |prefetch_service_|.
+  // This method is used to wrap PrefetchService::SetSuggestionProvider() to let
+  // our weak pointer guarantee everyone is still alive.
+  void SetSuggestionProvider();
+
   // Synchronously returns the offline id of the given page. The host will only
   // have knowledge of the page if it had previously returned status about it
   // through GetOfflineState() or as a notification. Otherwise the caller will

components/feed/content/feed_offline_host_unittest.cc

@@ -33,6 +33,7 @@ using offline_pages::StubOfflinePageModel;
 using offline_pages::MultipleOfflinePageItemCallback;
 using offline_pages::PrefetchSuggestion;
 using offline_pages::StubPrefetchService;
+using offline_pages::SuggestionsProvider;
 
 constexpr char kUrl1[] = "https://www.one.com/";
 constexpr char kUrl2[] = "https://www.two.com/";
@@ -42,6 +43,10 @@ constexpr char kOne[] = "One";
 constexpr char kTwo[] = "Two";
 constexpr char kThree[] = "Three";
 
+MATCHER_P(EqualsSpec, expected, "") {
+  return arg.spec() == expected;
+}
+
 class TestOfflinePageModel : public StubOfflinePageModel {
  public:
   void AddOfflinedPage(const std::string& url,
@@ -84,6 +89,13 @@ class TestOfflinePageModel : public StubOfflinePageModel {
   std::multimap<std::string, OfflinePageItem> url_to_offline_page_item_;
 };
 
+class TestPrefetchService : public StubPrefetchService {
+ public:
+  MOCK_METHOD1(SetSuggestionProvider, void(SuggestionsProvider*));
+  MOCK_METHOD0(NewSuggestionsAvailable, void());
+  MOCK_METHOD1(RemoveSuggestion, void(GURL));
+};
+
 void IgnoreStatus(std::vector<std::string> result) {}
 
 void CopyStatus(std::vector<std::string>* out,
@@ -102,7 +114,7 @@ class FeedOfflineHostTest : public ::testing::Test {
  public:
   TestOfflinePageModel* offline_page_model() { return &offline_page_model_; }
   FeedOfflineHost* host() { return host_.get(); }
-  StubPrefetchService* prefetch_service() { return &prefetch_service_; }
+  TestPrefetchService* prefetch_service() { return &prefetch_service_; }
   int get_suggestion_consumed_count() { return suggestion_consumed_count_; }
   int get_suggestions_shown_count() { return suggestions_shown_count_; }
   int get_get_known_content_count() { return get_known_content_count_; }
@@ -154,7 +166,7 @@ class FeedOfflineHostTest : public ::testing::Test {
 
   base::test::ScopedTaskEnvironment scoped_task_environment_;
   TestOfflinePageModel offline_page_model_;
-  StubPrefetchService prefetch_service_;
+  TestPrefetchService prefetch_service_;
   std::unique_ptr<FeedOfflineHost> host_;
   int suggestion_consumed_count_ = 0;
   int suggestions_shown_count_ = 0;
@@ -375,4 +387,37 @@ TEST_F(FeedOfflineHostTest, NoAddObserverWithoutInitialize) {
   ResetHost();
 }
 
+TEST_F(FeedOfflineHostTest, SetSuggestionProviderNotCalledSynchronously) {
+  EXPECT_CALL(*prefetch_service(), SetSuggestionProvider(testing::_)).Times(0);
+}
+
+TEST_F(FeedOfflineHostTest, SetSuggestionProviderCalledAsync) {
+  EXPECT_CALL(*prefetch_service(), SetSuggestionProvider(testing::_)).Times(1);
+  RunUntilIdle();
+}
+
+TEST_F(FeedOfflineHostTest, OnNewContentReceived) {
+  EXPECT_CALL(*prefetch_service(), NewSuggestionsAvailable()).Times(1);
+  host()->OnNewContentReceived();
+}
+
+TEST_F(FeedOfflineHostTest, RemoveZeroSuggestions) {
+  EXPECT_CALL(*prefetch_service(), RemoveSuggestion(testing::_)).Times(0);
+  host()->OnContentRemoved({});
+}
+
+TEST_F(FeedOfflineHostTest, RemoveOneSuggestion) {
+  EXPECT_CALL(*prefetch_service(), RemoveSuggestion(EqualsSpec(kUrl1)))
+      .Times(1);
+  host()->OnContentRemoved({kUrl1});
+}
+
+TEST_F(FeedOfflineHostTest, RemoveTwoSuggestions) {
+  EXPECT_CALL(*prefetch_service(), RemoveSuggestion(EqualsSpec(kUrl1)))
+      .Times(1);
+  EXPECT_CALL(*prefetch_service(), RemoveSuggestion(EqualsSpec(kUrl2)))
+      .Times(1);
+  host()->OnContentRemoved({kUrl1, kUrl2});
+}
+
 }  // namespace feed