Use base::WeakPtr for DirectFeedFetcher::Delegate call

brave · Dec 10, 2024 · 071fe11 · 071fe11
1 parent 9f3d6a8
commit 071fe11
Show file tree

Hide file tree

Showing 17 changed files with 65 additions and 45 deletions.
diff --git a/browser/brave_news/direct_feed_fetcher_delegate_impl.cc b/browser/brave_news/direct_feed_fetcher_delegate_impl.cc
@@ -17,6 +17,8 @@ DirectFeedFetcherDelegateImpl::DirectFeedFetcherDelegateImpl(
       https_upgrade_exceptions_service_(
           g_brave_browser_process->https_upgrade_exceptions_service()) {}
 
+DirectFeedFetcherDelegateImpl::~DirectFeedFetcherDelegateImpl() = default;
+
 bool DirectFeedFetcherDelegateImpl::ShouldUpgradeToHttps(const GURL& url) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (!host_content_settings_map_ || !https_upgrade_exceptions_service_) {
@@ -26,10 +28,9 @@ bool DirectFeedFetcherDelegateImpl::ShouldUpgradeToHttps(const GURL& url) {
                                              https_upgrade_exceptions_service_);
 }
 
-void DirectFeedFetcherDelegateImpl::Shutdown() {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  host_content_settings_map_ = nullptr;
-  https_upgrade_exceptions_service_ = nullptr;
+base::WeakPtr<DirectFeedFetcher::Delegate>
+DirectFeedFetcherDelegateImpl::AsWeakPtr() {
+  return weak_ptr_factory_.GetWeakPtr();
 }
 
 }  // namespace brave_news
diff --git a/browser/brave_news/direct_feed_fetcher_delegate_impl.h b/browser/brave_news/direct_feed_fetcher_delegate_impl.h
@@ -21,20 +21,23 @@ class DirectFeedFetcherDelegateImpl : public DirectFeedFetcher::Delegate {
  public:
   explicit DirectFeedFetcherDelegateImpl(
       HostContentSettingsMap* host_content_settings_map);
-  ~DirectFeedFetcherDelegateImpl() override = default;
+  ~DirectFeedFetcherDelegateImpl() override;
 
   DirectFeedFetcherDelegateImpl(const DirectFeedFetcherDelegateImpl&) = delete;
   DirectFeedFetcherDelegateImpl& operator=(
       const DirectFeedFetcherDelegateImpl&) = delete;
 
   // Must be called on UI thread
   bool ShouldUpgradeToHttps(const GURL& url) override;
-  void Shutdown() override;
+
+  base::WeakPtr<DirectFeedFetcher::Delegate> AsWeakPtr() override;
 
  private:
   raw_ptr<HostContentSettingsMap> host_content_settings_map_;
   raw_ptr<https_upgrade_exceptions::HttpsUpgradeExceptionsService>
       https_upgrade_exceptions_service_;
+
+  base::WeakPtrFactory<DirectFeedFetcherDelegateImpl> weak_ptr_factory_{this};
 };
 
 }  // namespace brave_news

diff --git a/components/brave_news/browser/brave_news_controller.cc b/components/brave_news/browser/brave_news_controller.cc
@@ -117,15 +117,14 @@ BraveNewsController::BraveNewsController(
                                   url_loader_factory),
       history_service_(history_service),
       url_loader_factory_(url_loader_factory),
-      task_runner_(base::ThreadPool::CreateSingleThreadTaskRunner(
-          {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
-           base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN})),
-      direct_feed_fetcher_delegate_(direct_feed_fetcher_delegate.release(),
-                                    base::OnTaskRunnerDeleter(task_runner_)),
+      direct_feed_fetcher_delegate_(std::move(direct_feed_fetcher_delegate)),
       pref_manager_(*prefs),
       news_metrics_(prefs, pref_manager_),
       direct_feed_controller_(url_loader_factory,
-                              direct_feed_fetcher_delegate_.get()),
+                              direct_feed_fetcher_delegate_->AsWeakPtr()),
+      task_runner_(base::ThreadPool::CreateSingleThreadTaskRunner(
+          {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
+           base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN})),
       engine_(nullptr, base::OnTaskRunnerDeleter(task_runner_)),
       initialization_promise_(
           3,
@@ -147,7 +146,6 @@ BraveNewsController::BraveNewsController(
 }
 
 BraveNewsController::~BraveNewsController() {
-  direct_feed_fetcher_delegate_->Shutdown();
   net::NetworkChangeNotifier::RemoveNetworkChangeObserver(this);
 }
 
@@ -800,9 +798,9 @@ void BraveNewsController::Prefetch() {
 }
 
 void BraveNewsController::ResetEngine() {
-  engine_.reset(new BraveNewsEngine(url_loader_factory_->Clone(),
-                                    MakeHistoryQuerier(),
-                                    direct_feed_fetcher_delegate_.get()));
+  engine_.reset(
+      new BraveNewsEngine(url_loader_factory_->Clone(), MakeHistoryQuerier(),
+                          direct_feed_fetcher_delegate_->AsWeakPtr()));
 }
 
 void BraveNewsController::ConditionallyStartOrStopTimer() {

diff --git a/components/brave_news/browser/brave_news_controller.h b/components/brave_news/browser/brave_news_controller.h
@@ -185,9 +185,7 @@ class BraveNewsController
   raw_ptr<history::HistoryService> history_service_;
   scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
 
-  scoped_refptr<base::SequencedTaskRunner> task_runner_;
-  std::unique_ptr<DirectFeedFetcher::Delegate, base::OnTaskRunnerDeleter>
-      direct_feed_fetcher_delegate_;
+  std::unique_ptr<DirectFeedFetcher::Delegate> direct_feed_fetcher_delegate_;
 
   BraveNewsPrefManager pref_manager_;
   SubscriptionsSnapshot last_subscriptions_;
@@ -196,6 +194,7 @@ class BraveNewsController
 
   DirectFeedController direct_feed_controller_;
 
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
   // Created on this sequence but lives on |task_runner_|.
   std::unique_ptr<BraveNewsEngine, base::OnTaskRunnerDeleter> engine_;
 

diff --git a/components/brave_news/browser/brave_news_engine.cc b/components/brave_news/browser/brave_news_engine.cc
@@ -33,7 +33,7 @@ BraveNewsEngine::BraveNewsEngine(
     std::unique_ptr<network::PendingSharedURLLoaderFactory>
         pending_shared_url_loader_factory,
     BackgroundHistoryQuerier history_querier,
-    DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate)
+    base::WeakPtr<DirectFeedFetcher::Delegate> direct_feed_fetcher_delegate)
     : pending_shared_url_loader_factory_(
           std::move(pending_shared_url_loader_factory)),
       history_querier_(std::move(history_querier)),

diff --git a/components/brave_news/browser/brave_news_engine.h b/components/brave_news/browser/brave_news_engine.h
@@ -51,7 +51,7 @@ class BraveNewsEngine {
       std::unique_ptr<network::PendingSharedURLLoaderFactory>
           pending_shared_url_loader_factory,
       BackgroundHistoryQuerier history_querier,
-      DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate);
+      base::WeakPtr<DirectFeedFetcher::Delegate> direct_feed_fetcher_delegate);
   BraveNewsEngine(const BraveNewsEngine&) = delete;
   BraveNewsEngine& operator=(const BraveNewsEngine&) = delete;
   ~BraveNewsEngine();
@@ -107,7 +107,7 @@ class BraveNewsEngine {
   BackgroundHistoryQuerier history_querier_
       GUARDED_BY_CONTEXT(sequence_checker_);
 
-  raw_ptr<DirectFeedFetcher::Delegate> direct_feed_fetcher_delegate_;
+  base::WeakPtr<DirectFeedFetcher::Delegate> direct_feed_fetcher_delegate_;
 
   std::unique_ptr<api_request_helper::APIRequestHelper> api_request_helper_
       GUARDED_BY_CONTEXT(sequence_checker_);

diff --git a/components/brave_news/browser/direct_feed_controller.cc b/components/brave_news/browser/direct_feed_controller.cc
@@ -37,7 +37,7 @@ DirectFeedController::FindFeedRequest::~FindFeedRequest() = default;
 
 DirectFeedController::DirectFeedController(
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
-    DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate)
+    base::WeakPtr<DirectFeedFetcher::Delegate> direct_feed_fetcher_delegate)
     : fetcher_(url_loader_factory, direct_feed_fetcher_delegate) {}
 
 DirectFeedController::~DirectFeedController() = default;

diff --git a/components/brave_news/browser/direct_feed_controller.h b/components/brave_news/browser/direct_feed_controller.h
@@ -30,7 +30,7 @@ class DirectFeedController {
  public:
   explicit DirectFeedController(
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
-      DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate);
+      base::WeakPtr<DirectFeedFetcher::Delegate> direct_feed_fetcher_delegate);
   ~DirectFeedController();
   DirectFeedController(const DirectFeedController&) = delete;
   DirectFeedController& operator=(const DirectFeedController&) = delete;

diff --git a/components/brave_news/browser/direct_feed_controller_unittest.cc b/components/brave_news/browser/direct_feed_controller_unittest.cc
@@ -78,10 +78,8 @@ constexpr char kFeedURL[] = "https://example.com/feed";
 class BraveNewsDirectFeedControllerTest : public testing::Test {
  public:
   BraveNewsDirectFeedControllerTest()
-      : direct_feed_controller_(
-
-            test_url_loader_factory_.GetSafeWeakWrapper(),
-            &direct_feed_fetcher_delegate_) {}
+      : direct_feed_controller_(test_url_loader_factory_.GetSafeWeakWrapper(),
+                                direct_feed_fetcher_delegate_.AsWeakPtr()) {}
 
  protected:
   class MockDirectFeedFetcherDelegate : public DirectFeedFetcher::Delegate {
@@ -90,7 +88,12 @@ class BraveNewsDirectFeedControllerTest : public testing::Test {
 
     bool ShouldUpgradeToHttps(const GURL& url) override { return true; }
 
-    void Shutdown() override {}
+    base::WeakPtr<DirectFeedFetcher::Delegate> AsWeakPtr() override {
+      return weak_ptr_factory_.GetWeakPtr();
+    }
+
+   private:
+    base::WeakPtrFactory<MockDirectFeedFetcherDelegate> weak_ptr_factory_{this};
   };
 
   std::tuple<bool, std::string> VerifyFeedUrl(GURL feed_url) {

diff --git a/components/brave_news/browser/direct_feed_fetcher.cc b/components/brave_news/browser/direct_feed_fetcher.cc
@@ -147,21 +147,37 @@ DirectFeedResponse::DirectFeedResponse(DirectFeedResponse&&) = default;
 
 DirectFeedFetcher::DirectFeedFetcher(
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
-    Delegate* delegate)
+    base::WeakPtr<Delegate> delegate)
     : url_loader_factory_(url_loader_factory), delegate_(delegate) {}
 DirectFeedFetcher::~DirectFeedFetcher() = default;
 
 void DirectFeedFetcher::DownloadFeed(GURL url,
                                      std::string publisher_id,
                                      DownloadFeedCallback callback) {
   if (url.SchemeIs(url::kHttpScheme)) {
-    content::GetUIThreadTaskRunner({})->PostTaskAndReplyWithResult(
+    content::GetUIThreadTaskRunner({})->PostTask(
         FROM_HERE,
-        base::BindOnce(&Delegate::ShouldUpgradeToHttps,
-                       base::Unretained(delegate_), url),
-        base::BindOnce(&DirectFeedFetcher::DownloadFeedHelper,
-                       weak_ptr_factory_.GetWeakPtr(), url, publisher_id,
-                       std::move(callback)));
+        base::BindOnce(
+            [](base::WeakPtr<Delegate> delegate,
+               scoped_refptr<base::SingleThreadTaskRunner> source_task_runner,
+               base::OnceCallback<void(bool)> callback, GURL url) {
+              if (delegate.WasInvalidated()) {
+                return;
+              }
+              bool should_upgrade = delegate->ShouldUpgradeToHttps(url);
+              source_task_runner->PostTask(
+                  FROM_HERE,
+                  base::BindOnce(
+                      [](base::OnceCallback<void(bool)> callback, bool result) {
+                        std::move(callback).Run(result);
+                      },
+                      std::move(callback), should_upgrade));
+            },
+            delegate_, base::SingleThreadTaskRunner::GetCurrentDefault(),
+            base::BindOnce(&DirectFeedFetcher::DownloadFeedHelper,
+                           weak_ptr_factory_.GetWeakPtr(), url, publisher_id,
+                           std::move(callback)),
+            url));
     return;
   }
   DownloadFeedHelper(url, publisher_id, std::move(callback), false);

diff --git a/components/brave_news/browser/direct_feed_fetcher.h b/components/brave_news/browser/direct_feed_fetcher.h
@@ -72,12 +72,12 @@ class DirectFeedFetcher {
     virtual ~Delegate() = default;
 
     virtual bool ShouldUpgradeToHttps(const GURL& url) = 0;
-    virtual void Shutdown();
+    virtual base::WeakPtr<DirectFeedFetcher::Delegate> AsWeakPtr() = 0;
   };
 
   explicit DirectFeedFetcher(
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
-      Delegate* delegate);
+      base::WeakPtr<Delegate> delegate);
   DirectFeedFetcher(const DirectFeedFetcher&) = delete;
   DirectFeedFetcher& operator=(const DirectFeedFetcher&) = delete;
   ~DirectFeedFetcher();
@@ -110,7 +110,7 @@ class DirectFeedFetcher {
 
   scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
 
-  raw_ptr<Delegate> delegate_;
+  base::WeakPtr<Delegate> delegate_;
 
   base::WeakPtrFactory<DirectFeedFetcher> weak_ptr_factory_{this};
 };

diff --git a/components/brave_news/browser/feed_controller.cc b/components/brave_news/browser/feed_controller.cc
@@ -35,7 +35,7 @@ FeedController::FeedController(
     PublishersController* publishers_controller,
     BackgroundHistoryQuerier& history_querier,
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
-    DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate)
+    base::WeakPtr<DirectFeedFetcher::Delegate> direct_feed_fetcher_delegate)
     : publishers_controller_(publishers_controller),
       history_querier_(history_querier),
       feed_fetcher_(*publishers_controller,

diff --git a/components/brave_news/browser/feed_controller.h b/components/brave_news/browser/feed_controller.h
@@ -38,7 +38,7 @@ class FeedController {
       PublishersController* publishers_controller,
       BackgroundHistoryQuerier& history_querier,
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
-      DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate);
+      base::WeakPtr<DirectFeedFetcher::Delegate> direct_feed_fetcher_delegate);
   ~FeedController();
   FeedController(const FeedController&) = delete;
   FeedController& operator=(const FeedController&) = delete;

diff --git a/components/brave_news/browser/feed_fetcher.cc b/components/brave_news/browser/feed_fetcher.cc
@@ -106,7 +106,7 @@ std::tuple<FeedItems, ETags> FeedFetcher::CombineFeedSourceResults(
 FeedFetcher::FeedFetcher(
     PublishersController& publishers_controller,
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
-    DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate)
+    base::WeakPtr<DirectFeedFetcher::Delegate> direct_feed_fetcher_delegate)
     : publishers_controller_(publishers_controller),
       api_request_helper_(GetNetworkTrafficAnnotationTag(), url_loader_factory),
       direct_feed_fetcher_(url_loader_factory, direct_feed_fetcher_delegate) {}

diff --git a/components/brave_news/browser/feed_fetcher.h b/components/brave_news/browser/feed_fetcher.h
@@ -30,7 +30,7 @@ class FeedFetcher {
  public:
   FeedFetcher(PublishersController& publishers_controller,
               scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
-              DirectFeedFetcher::Delegate* delegate);
+              base::WeakPtr<DirectFeedFetcher::Delegate> delegate);
   ~FeedFetcher();
   FeedFetcher(const FeedFetcher&) = delete;
   FeedFetcher& operator=(const FeedFetcher&) = delete;

diff --git a/components/brave_news/browser/feed_v2_builder.cc b/components/brave_news/browser/feed_v2_builder.cc
@@ -614,7 +614,7 @@ FeedV2Builder::FeedV2Builder(
     SuggestionsController& suggestions_controller,
     BackgroundHistoryQuerier& history_querier,
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
-    DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate)
+    base::WeakPtr<DirectFeedFetcher::Delegate> direct_feed_fetcher_delegate)
     : publishers_controller_(publishers_controller),
       channels_controller_(channels_controller),
       suggestions_controller_(suggestions_controller),

diff --git a/components/brave_news/browser/feed_v2_builder.h b/components/brave_news/browser/feed_v2_builder.h
@@ -45,7 +45,7 @@ class FeedV2Builder {
       SuggestionsController& suggestions_controller,
       BackgroundHistoryQuerier& history_querier,
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
-      DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate);
+      base::WeakPtr<DirectFeedFetcher::Delegate> direct_feed_fetcher_delegate);
   FeedV2Builder(const FeedV2Builder&) = delete;
   FeedV2Builder& operator=(const FeedV2Builder&) = delete;
   ~FeedV2Builder();