When an article is dismissed by feed, remove the offline page

And prevent it from being downloaded later.

Note that I manually tested that this does work.

Change-Id: I3b445f1a5fe8fa675ac9c7addc96b29eb8da12ed
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1536606
Commit-Queue: Dan H <harringtond@chromium.org>
Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#659065}

components/offline_pages/core/prefetch/BUILD.gn

@@ -90,6 +90,8 @@ static_library("prefetch") {
     "tasks/metrics_finalization_task.h",
     "tasks/page_bundle_update_task.cc",
     "tasks/page_bundle_update_task.h",
+    "tasks/remove_url_task.cc",
+    "tasks/remove_url_task.h",
     "tasks/sent_get_operation_cleanup_task.cc",
     "tasks/sent_get_operation_cleanup_task.h",
     "tasks/stale_entry_finalizer_task.cc",

components/offline_pages/core/prefetch/prefetch_dispatcher_impl.cc

@@ -44,6 +44,7 @@
 #include "components/offline_pages/core/prefetch/tasks/mark_operation_done_task.h"
 #include "components/offline_pages/core/prefetch/tasks/metrics_finalization_task.h"
 #include "components/offline_pages/core/prefetch/tasks/page_bundle_update_task.h"
+#include "components/offline_pages/core/prefetch/tasks/remove_url_task.h"
 #include "components/offline_pages/core/prefetch/tasks/sent_get_operation_cleanup_task.h"
 #include "components/offline_pages/core/prefetch/tasks/stale_entry_finalizer_task.h"
 #include "components/offline_pages/core/prefetch/thumbnail_fetcher.h"
@@ -146,8 +147,17 @@ void PrefetchDispatcherImpl::NewSuggestionsAvailable(
 void PrefetchDispatcherImpl::RemoveSuggestion(const GURL& url) {
   if (!prefetch_prefs::IsEnabled(pref_service_))
     return;
-  // TODO(https://crbug.com/841516): to be implemented soon.
-  NOTIMPLEMENTED();
+
+  // Remove the URL from the prefetch database.
+  task_queue_.AddTask(MakeRemoveUrlTask(service_->GetPrefetchStore(), url));
+
+  // Remove the URL from the offline model.
+  PageCriteria criteria;
+  criteria.url = url;
+  criteria.client_namespaces =
+      std::vector<std::string>{kSuggestedArticlesNamespace};
+  service_->GetOfflinePageModel()->DeletePagesWithCriteria(criteria,
+                                                           base::DoNothing());
 }
 
 void PrefetchDispatcherImpl::RemoveAllUnprocessedPrefetchURLs(

components/offline_pages/core/prefetch/prefetch_dispatcher_impl_unittest.cc

@@ -166,6 +166,8 @@ class MockOfflinePageModel : public StubOfflinePageModel {
 
   MOCK_METHOD2(AddPage,
                void(const OfflinePageItem& page, AddPageCallback callback));
+  MOCK_METHOD2(DeletePagesWithCriteria,
+               void(const PageCriteria& criteria, DeletePageCallback callback));
 
   void StoreThumbnail(int64_t offline_id, std::string thumbnail) override {
     insert_or_update_visuals(offline_id, thumbnail, std::string());
@@ -425,6 +427,13 @@ class PrefetchDispatcherTest : public PrefetchRequestTestBase {
   PrefetchService* prefetch_service() { return taco_->prefetch_service(); }
   TestDownloadService* download_service() { return taco_->download_service(); }
 
+  // Asserts that there exists a single item in the database, and returns it.
+  PrefetchItem GetSingleItem() {
+    std::set<PrefetchItem> items;
+    EXPECT_EQ(1ul, store_util_.GetAllItems(&items));
+    return *items.begin();
+  }
+
  protected:
   // Owned by |taco_|.
   MockOfflinePageModel* offline_model_;
@@ -1069,4 +1078,50 @@ TEST_F(PrefetchDispatcherTest, FeedPrefetchItemFlow) {
   EXPECT_EQ(PrefetchItemErrorCode::SUCCESS, item.error_code);
 }
 
+// Tests that |RemoveSuggestion()| removes items from the offline database, and
+// triggers finalization of the prefetch item.
+TEST_F(PrefetchDispatcherTest, RemoveSuggestion) {
+  Configure(PrefetchServiceTestTaco::kFeed);
+
+  EXPECT_CALL(*offline_model_, DeletePagesWithCriteria(_, _))
+      .WillOnce([&](const PageCriteria& criteria, DeletePageCallback callback) {
+        EXPECT_EQ(TestSuggestion1().article_url, criteria.url);
+        EXPECT_EQ(std::vector<std::string>({kSuggestedArticlesNamespace}),
+                  criteria.client_namespaces);
+      });
+
+  suggestions_provider_->SetSuggestions({TestSuggestion1()});
+  prefetch_service()->NewSuggestionsAvailable();
+  RunUntilIdle();
+
+  const PrefetchItem item_state_1 = GetSingleItem();
+
+  dispatcher()->RemoveSuggestion(TestSuggestion1().article_url);
+  RunUntilIdle();
+  const PrefetchItem item_state_2 = GetSingleItem();
+
+  // The item is initially not finished.
+  EXPECT_EQ(TestSuggestion1().article_url, item_state_1.url);
+  EXPECT_NE(PrefetchItemState::FINISHED, item_state_1.state);
+
+  // The item is finished after the suggestion is removed.
+  EXPECT_EQ(PrefetchItemState::FINISHED, item_state_2.state);
+}
+
+// Verify that we can attempt to remove a URL that isn't in the prefetch
+// database.
+TEST_F(PrefetchDispatcherTest, RemoveSuggestionDoesNotExist) {
+  Configure(PrefetchServiceTestTaco::kFeed);
+
+  suggestions_provider_->SetSuggestions({TestSuggestion1()});
+  prefetch_service()->NewSuggestionsAvailable();
+  RunUntilIdle();
+
+  dispatcher()->RemoveSuggestion(GURL("http://otherurl.com"));
+  RunUntilIdle();
+
+  // Verify the item still exists.
+  GetSingleItem();
+}
+
 }  // namespace offline_pages

components/offline_pages/core/prefetch/tasks/remove_url_task.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/offline_pages/core/prefetch/tasks/remove_url_task.h"
+
+#include "components/offline_pages/core/prefetch/prefetch_types.h"
+#include "components/offline_pages/core/prefetch/store/prefetch_store.h"
+#include "sql/database.h"
+#include "sql/statement.h"
+
+namespace offline_pages {
+
+namespace {
+
+bool RemoveURL(const GURL& url, sql::Database* db) {
+  static const char kSql[] =
+      "UPDATE prefetch_items SET state=?,error_code=?"
+      " WHERE requested_url=?"
+      " AND state NOT IN (?,?)";
+
+  sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
+  statement.BindInt(0, static_cast<int>(PrefetchItemState::FINISHED));
+  statement.BindInt(
+      1, static_cast<int>(PrefetchItemErrorCode::SUGGESTION_INVALIDATED));
+  statement.BindString(2, url.spec());
+  statement.BindInt(3, static_cast<int>(PrefetchItemState::FINISHED));
+  statement.BindInt(4, static_cast<int>(PrefetchItemState::ZOMBIE));
+  return statement.Run();
+}
+
+}  // namespace
+
+std::unique_ptr<SqlCallbackTask> MakeRemoveUrlTask(PrefetchStore* store,
+                                                   const GURL& url) {
+  return std::make_unique<SqlCallbackTask>(store,
+                                           base::BindOnce(&RemoveURL, url));
+}
+
+}  // namespace offline_pages

components/offline_pages/core/prefetch/tasks/remove_url_task.h

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_OFFLINE_PAGES_CORE_PREFETCH_TASKS_REMOVE_URL_TASK_H_
+#define COMPONENTS_OFFLINE_PAGES_CORE_PREFETCH_TASKS_REMOVE_URL_TASK_H_
+
+#include <string>
+#include <vector>
+
+#include "base/memory/weak_ptr.h"
+#include "components/offline_pages/task/sql_callback_task.h"
+#include "components/offline_pages/task/task.h"
+#include "url/gurl.h"
+
+namespace offline_pages {
+class PrefetchStore;
+
+// Creates a task that removes a URL from the pipeline. Finalizes any item with
+// matching URL if it's not already finalized.
+std::unique_ptr<SqlCallbackTask> MakeRemoveUrlTask(PrefetchStore* store,
+                                                   const GURL& url);
+
+}  // namespace offline_pages
+
+#endif  // COMPONENTS_OFFLINE_PAGES_CORE_PREFETCH_TASKS_REMOVE_URL_TASK_H_

components/offline_pages/task/BUILD.gn

@@ -10,6 +10,8 @@ static_library("task") {
   sources = [
     "closure_task.cc",
     "closure_task.h",
+    "sql_callback_task.cc",
+    "sql_callback_task.h",
     "sql_store_base.cc",
     "sql_store_base.h",
     "task.cc",

components/offline_pages/task/sql_callback_task.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+#include "components/offline_pages/task/sql_callback_task.h"
+
+#include "components/offline_pages/task/sql_store_base.h"
+
+namespace offline_pages {
+
+using ExecuteCallback = SqlCallbackTask::ExecuteCallback;
+
+SqlCallbackTask::SqlCallbackTask(SqlStoreBase* store,
+                                 ExecuteCallback exec_callback,
+                                 DoneCallback done_callback)
+    : store_(store),
+      exec_callback_(std::move(exec_callback)),
+      done_callback_(std::move(done_callback)) {}
+
+SqlCallbackTask::~SqlCallbackTask() = default;
+
+void SqlCallbackTask::Run() {
+  // Execute() requires that the callback returns a value, so we use
+  // ExecuteAndReturn to return a dummy boolean which is ignored.
+  store_->Execute(std::move(exec_callback_),
+                  base::BindOnce(&SqlCallbackTask::Done, GetWeakPtr()), false);
+}
+
+void SqlCallbackTask::Done(bool result) {
+  TaskComplete();
+  if (done_callback_)
+    std::move(done_callback_).Run(result);
+}
+
+}  // namespace offline_pages

components/offline_pages/task/sql_callback_task.h

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_OFFLINE_PAGES_TASK_SQL_CALLBACK_TASK_H_
+#define COMPONENTS_OFFLINE_PAGES_TASK_SQL_CALLBACK_TASK_H_
+
+#include "base/callback.h"
+#include "base/memory/weak_ptr.h"
+#include "components/offline_pages/task/task.h"
+
+namespace sql {
+class Database;
+}
+
+namespace offline_pages {
+class SqlStoreBase;
+
+// A simple task that calls store->Execute() with the provided |exec_callback|
+// and completes. |done_callback|, if provided, is called with the result. This
+// class can be used if there are no UI thread actions that need done.
+class SqlCallbackTask : public Task {
+ public:
+  typedef base::OnceCallback<bool(sql::Database* db)> ExecuteCallback;
+  typedef base::OnceCallback<void(bool)> DoneCallback;
+
+  SqlCallbackTask(SqlStoreBase* store,
+                  ExecuteCallback exec_callback,
+                  DoneCallback done_callback = {});
+  ~SqlCallbackTask() override;
+  void Run() override;
+
+ private:
+  base::WeakPtr<SqlCallbackTask> GetWeakPtr() {
+    return weak_ptr_factory_.GetWeakPtr();
+  }
+
+  void Done(bool result);
+
+  SqlStoreBase* store_;
+  ExecuteCallback exec_callback_;
+  DoneCallback done_callback_;
+  base::WeakPtrFactory<SqlCallbackTask> weak_ptr_factory_{this};
+};
+
+}  // namespace offline_pages
+
+#endif  // COMPONENTS_OFFLINE_PAGES_TASK_SQL_CALLBACK_TASK_H_