From a2e246a0af28498d0606c2373a250afc2b56aa65 Mon Sep 17 00:00:00 2001 From: Jay Harris Date: Thu, 8 Feb 2024 12:47:56 +1300 Subject: [PATCH] [Brave News]: Build feed off the main thread (#21833) --- .../api_request_helper/api_request_helper.h | 1 + components/brave_news/browser/DEPS | 3 +- components/brave_news/browser/feed_fetcher.cc | 117 ++-- components/brave_news/browser/feed_fetcher.h | 3 + .../brave_news/browser/feed_v2_builder.cc | 576 ++++++++++-------- .../brave_news/browser/feed_v2_builder.h | 25 +- 6 files changed, 428 insertions(+), 297 deletions(-) diff --git a/components/api_request_helper/api_request_helper.h b/components/api_request_helper/api_request_helper.h index d925d52ac071..fb0f5e02cba5 100644 --- a/components/api_request_helper/api_request_helper.h +++ b/components/api_request_helper/api_request_helper.h @@ -58,6 +58,7 @@ class APIRequestResult { const std::string& body() const { return body_; } // `base::Value` of sanitized json response. const base::Value& value_body() const { return value_body_; } + base::Value& value_body() { return value_body_; } // HTTP response headers. const base::flat_map& headers() const { return headers_; diff --git a/components/brave_news/browser/DEPS b/components/brave_news/browser/DEPS index f86edfe1941d..a4be04ae47f4 100644 --- a/components/brave_news/browser/DEPS +++ b/components/brave_news/browser/DEPS @@ -1,6 +1,7 @@ include_rules = [ "+net", + "+content/public/browser/browser_thread.h", "+services/network/public", "+services/network/public/mojom", "+third_party/re2", -] \ No newline at end of file +] diff --git a/components/brave_news/browser/feed_fetcher.cc b/components/brave_news/browser/feed_fetcher.cc index adaf621dbcff..a460b91c9f8f 100644 --- a/components/brave_news/browser/feed_fetcher.cc +++ b/components/brave_news/browser/feed_fetcher.cc @@ -7,7 +7,9 @@ #include #include -#include +#include +#include +#include #include #include @@ -17,8 +19,9 @@ #include "base/functional/callback_forward.h" #include "base/location.h" #include "base/memory/weak_ptr.h" -#include "base/one_shot_event.h" #include "base/ranges/algorithm.h" +#include "base/task/thread_pool.h" +#include "base/values.h" #include "brave/components/api_request_helper/api_request_helper.h" #include "brave/components/brave_news/browser/channels_controller.h" #include "brave/components/brave_news/browser/combined_feed_parsing.h" @@ -56,6 +59,51 @@ FeedFetcher::FeedSourceResult::~FeedSourceResult() = default; FeedFetcher::FeedSourceResult::FeedSourceResult( FeedFetcher::FeedSourceResult&&) = default; +// static +std::tuple FeedFetcher::CombineFeedSourceResults( + std::vector results) { + std::size_t total_size = 0; + for (const auto& result : results) { + total_size += result.items.size(); + } + VLOG(1) << "All feed item fetches done with item count: " << total_size; + + ETags etags; + FeedItems feed; + feed.reserve(total_size); + + // We want to deduplicate the feed, as the feeds for different + // regions **may** have overlap. + std::unordered_set seen; + + // reserve |total_size| space in |seen|. This is more than we'll + // likely need but should be in the correct ballpark. + seen.reserve(total_size); + + for (auto& result : results) { + etags[result.key] = result.etag; + for (auto& item : result.items) { + GURL url; + if (item->is_article()) { + url = item->get_article()->data->url; + } else if (item->is_promoted_article()) { + url = item->get_promoted_article()->data->url; + } + + // Skip this, we've already seen it. + auto spec = url.spec(); + if (!url.is_empty() && seen.contains(spec)) { + continue; + } + seen.insert(std::move(spec)); + + feed.push_back(std::move(item)); + } + } + + return std::make_tuple(std::move(feed), std::move(etags)); +} + FeedFetcher::FeedFetcher( PublishersController& publishers_controller, ChannelsController& channels_controller, @@ -152,47 +200,40 @@ void FeedFetcher::OnFetchFeedFetchedFeed( return; } - std::move(callback).Run({locale, etag, ParseFeedItems(result.value_body())}); + base::ThreadPool::PostTaskAndReplyWithResult( + FROM_HERE, + base::BindOnce(&ParseFeedItems, std::move(result.value_body())), + base::BindOnce( + [](base::WeakPtr fetcher, std::string locale, + std::string etag, FetchFeedSourceCallback callback, + std::vector items) { + // If the fetcher was destroyed, don't run the callback. + if (!fetcher) { + return; + } + std::move(callback).Run( + {std::move(locale), std::move(etag), std::move(items)}); + }, + weak_ptr_factory_.GetWeakPtr(), std::move(locale), std::move(etag), + std::move(callback))); } void FeedFetcher::OnFetchFeedFetchedAll(FetchFeedCallback callback, Publishers publishers, std::vector results) { - std::size_t total_size = 0; - for (const auto& result : results) { - total_size += result.items.size(); - } - VLOG(1) << "All feed item fetches done with item count: " << total_size; - - ETags etags; - FeedItems feed; - feed.reserve(total_size); - - // We want to deduplicate the feed, as the feeds for different regions **may** - // have overlap. - base::flat_set seen; - - for (auto& result : results) { - etags[result.key] = result.etag; - for (auto& item : result.items) { - GURL url; - if (item->is_article()) { - url = item->get_article()->data->url; - } else if (item->is_promoted_article()) { - url = item->get_promoted_article()->data->url; - } - - // Skip this, we've already seen it. - if (!url.is_empty() && seen.contains(url)) { - continue; - } - seen.insert(url); - - feed.push_back(std::move(item)); - } - } - - std::move(callback).Run(std::move(feed), std::move(etags)); + base::ThreadPool::PostTaskAndReplyWithResult( + FROM_HERE, base::BindOnce(&CombineFeedSourceResults, std::move(results)), + base::BindOnce( + [](base::WeakPtr fetcher, FetchFeedCallback callback, + std::tuple result) { + // If we've been destroyed, don't run the callback. + if (!fetcher) { + return; + } + std::move(callback).Run(std::move(std::get<0>(result)), + std::move(std::get<1>(result))); + }, + weak_ptr_factory_.GetWeakPtr(), std::move(callback))); } void FeedFetcher::IsUpdateAvailable(ETags etags, diff --git a/components/brave_news/browser/feed_fetcher.h b/components/brave_news/browser/feed_fetcher.h index 1d34b12afb6b..c5a1f7442cdb 100644 --- a/components/brave_news/browser/feed_fetcher.h +++ b/components/brave_news/browser/feed_fetcher.h @@ -8,6 +8,7 @@ #include #include +#include #include #include "base/containers/flat_map.h" @@ -55,6 +56,8 @@ class FeedFetcher { }; using FetchFeedSourceCallback = base::OnceCallback; + static std::tuple CombineFeedSourceResults( + std::vector results); // Steps for |FetchFeed| void OnFetchFeedFetchedPublishers(FetchFeedCallback callback, diff --git a/components/brave_news/browser/feed_v2_builder.cc b/components/brave_news/browser/feed_v2_builder.cc index f8fc56a15d8d..a1184ff69073 100644 --- a/components/brave_news/browser/feed_v2_builder.cc +++ b/components/brave_news/browser/feed_v2_builder.cc @@ -19,23 +19,25 @@ #include "base/functional/bind.h" #include "base/functional/callback_forward.h" #include "base/functional/callback_helpers.h" +#include "base/location.h" #include "base/logging.h" #include "base/memory/scoped_refptr.h" #include "base/memory/weak_ptr.h" #include "base/rand_util.h" #include "base/ranges/algorithm.h" #include "base/strings/string_number_conversions.h" +#include "base/task/thread_pool.h" #include "base/time/time.h" +#include "brave/components/brave_news/api/topics.h" #include "brave/components/brave_news/browser/channels_controller.h" #include "brave/components/brave_news/browser/feed_fetcher.h" #include "brave/components/brave_news/browser/publishers_controller.h" #include "brave/components/brave_news/browser/signal_calculator.h" #include "brave/components/brave_news/browser/topics_fetcher.h" -#include "brave/components/brave_news/common/brave_news.mojom-forward.h" -#include "brave/components/brave_news/common/brave_news.mojom-shared.h" #include "brave/components/brave_news/common/brave_news.mojom.h" #include "brave/components/brave_news/common/features.h" #include "components/prefs/pref_service.h" +#include "content/public/browser/browser_thread.h" #include "mojo/public/cpp/bindings/pending_remote.h" #include "services/network/public/cpp/shared_url_loader_factory.h" #include "third_party/abseil-cpp/absl/types/optional.h" @@ -771,6 +773,56 @@ std::vector GenerateSpecialBlock( } // namespace +struct FeedV2Builder::FeedGenerationInfo { + std::string locale; + FeedItems feed_items; + Publishers publishers; + std::vector channels; + Signals signals; + std::vector suggested_publisher_ids; + TopicsResult topics; + FeedGenerationInfo(const std::string& locale, + const FeedItems& feed_items, + const Publishers& publishers, + std::vector channels, + const Signals& signals, + const std::vector& suggested_publisher_ids, + const TopicsResult& topics) + : locale(locale), + channels(std::move(channels)), + suggested_publisher_ids(suggested_publisher_ids) { + this->feed_items.reserve(feed_items.size()); + for (const auto& item : feed_items) { + this->feed_items.push_back(item->Clone()); + } + + this->publishers.reserve(publishers.size()); + for (const auto& [id, publisher] : publishers) { + this->publishers[id] = publisher.Clone(); + } + + this->signals.reserve(signals.size()); + for (const auto& [id, signal] : signals) { + this->signals[id] = signal->Clone(); + } + + this->topics.reserve(topics.size()); + for (const auto& topic : topics) { + std::vector articles; + articles.reserve(topic.second.size()); + for (const auto& article : topic.second) { + articles.push_back(article.Clone()); + } + this->topics.emplace_back(topic.first.Clone(), std::move(articles)); + } + } + FeedGenerationInfo(const FeedGenerationInfo&) = delete; + FeedGenerationInfo& operator=(const FeedGenerationInfo&) = delete; + FeedGenerationInfo(FeedGenerationInfo&&) = default; + FeedGenerationInfo& operator=(FeedGenerationInfo&&) = default; + ~FeedGenerationInfo() = default; +}; + FeedV2Builder::UpdateRequest::UpdateRequest(UpdateSettings settings, UpdateCallback callback) : settings(std::move(settings)) { @@ -799,6 +851,163 @@ void FeedV2Builder::UpdateRequest::AlsoUpdate( callbacks.push_back(std::move(callback)); } +// static +mojom::FeedV2Ptr FeedV2Builder::GenerateBasicFeed(FeedGenerationInfo info, + PickArticles pick_hero, + PickArticles pick_article) { + DVLOG(1) << __FUNCTION__; + DCHECK(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); + + auto articles = GetArticleInfos(info.locale, info.feed_items, info.publishers, + info.signals); + + auto feed = mojom::FeedV2::New(); + + constexpr size_t kIterationsPerAd = 2; + size_t blocks = 0; + while (!articles.empty()) { + auto items = GenerateBlock(articles, pick_hero, pick_article, + /*inline_discovery_ratio=*/0); + if (items.empty()) { + break; + } + + // After the first block, every second block should be an ad. + if (blocks % kIterationsPerAd == 0 && blocks != 0) { + std::ranges::move(GenerateSpecialBlock(info.suggested_publisher_ids), + std::back_inserter(items)); + } + + std::ranges::move(items, std::back_inserter(feed->items)); + blocks++; + } + + // Insert an ad as the second item. + if (feed->items.size() > 1) { + auto ad = GenerateAd(); + feed->items.insert(feed->items.begin() + 1, + std::make_move_iterator(ad.begin()), + std::make_move_iterator(ad.end())); + } + + return feed; +} + +// static +mojom::FeedV2Ptr FeedV2Builder::GenerateAllFeed(FeedGenerationInfo info) { + DVLOG(1) << __FUNCTION__; + DCHECK(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); + // Make a copy of these - we're going to edit the copy to prevent duplicates. + auto articles = GetArticleInfos(info.locale, info.feed_items, info.publishers, + info.signals); + auto feed = mojom::FeedV2::New(); + + base::span topics_span = base::make_span(info.topics); + + auto add_items = [&feed](std::vector& items) { + base::ranges::move(items, std::back_inserter(feed->items)); + }; + + std::vector eligible_content_groups; + for (const auto& channel_id : info.channels) { + eligible_content_groups.push_back(std::make_pair(channel_id, true)); + } + for (const auto& [publisher_id, publisher] : info.publishers) { + if (publisher->user_enabled_status == mojom::UserEnabled::ENABLED || + publisher->type == mojom::PublisherType::DIRECT_SOURCE) { + eligible_content_groups.push_back(std::make_pair(publisher_id, false)); + DVLOG(1) << "Subscribed to publisher: " << publisher->publisher_name; + } + } + + // If we aren't subscribed to anything, or we failed to fetch any articles + // from the internet, don't try and generate a feed. + if (eligible_content_groups.size() == 0 || info.feed_items.size() == 0) { + return feed; + } + + // Step 1: Generate a block + // https://docs.google.com/document/d/1bSVHunwmcHwyQTpa3ab4KRbGbgNQ3ym_GHvONnrBypg/edit#heading=h.rkq699fwps0 + std::vector initial_block = + GenerateBlockFromContentGroups(articles, info.locale, info.publishers, + eligible_content_groups); + DVLOG(1) << "Step 1: Standard Block (" << initial_block.size() + << " articles)"; + add_items(initial_block); + + // Step 2: We always add an advertisment after the first block. + // https://docs.google.com/document/d/1bSVHunwmcHwyQTpa3ab4KRbGbgNQ3ym_GHvONnrBypg/edit#heading=h.82154jsxm16 + auto advert = GenerateAd(); + DVLOG(1) << "Step 2: Advertisement"; + add_items(advert); + + // Step 3: Generate a top news block + // https://docs.google.com/document/d/1bSVHunwmcHwyQTpa3ab4KRbGbgNQ3ym_GHvONnrBypg/edit#heading=h.7z05nb4b269d + // This block is a bit special - we take the top articles from the top few + // topics and display them in a cluster. If there are no topics, we try and do + // the same thing, but with the Top News channel. + auto top_news_block = GenerateTopTopicsBlock(info.publishers, topics_span); + if (top_news_block.empty()) { + top_news_block = GenerateChannelBlock(info.locale, info.publishers, + kTopNewsChannel, articles); + } + DVLOG(1) << "Step 3: Top News Block"; + add_items(top_news_block); + + // Repeat step 4 - 6 until we don't have any more articles to add to the feed. + constexpr uint8_t kIterationTypes = 3; + uint32_t iteration = 0; + while (true) { + std::vector items; + + auto iteration_type = iteration % kIterationTypes; + + // Step 4: Block Generation + // https://docs.google.com/document/d/1bSVHunwmcHwyQTpa3ab4KRbGbgNQ3ym_GHvONnrBypg/edit#heading=h.os2ze8cesd8v + if (iteration_type == 0) { + DVLOG(1) << "Step 4: Standard Block"; + items = GenerateBlockFromContentGroups( + articles, info.locale, info.publishers, eligible_content_groups); + } else if (iteration_type == 1) { + // Step 5: Block or Cluster Generation + // https://docs.google.com/document/d/1bSVHunwmcHwyQTpa3ab4KRbGbgNQ3ym_GHvONnrBypg/edit#heading=h.tpvsjkq0lzmy + // Half the time, a normal block + if (TossCoin()) { + DVLOG(1) << "Step 5: Standard Block"; + items = GenerateBlockFromContentGroups( + articles, info.locale, info.publishers, eligible_content_groups); + } else { + items = GenerateClusterBlock(info.locale, info.publishers, + info.channels, topics_span, articles); + } + } else if (iteration_type == 2) { + // Step 6: Optional special card + // https://docs.google.com/document/d/1bSVHunwmcHwyQTpa3ab4KRbGbgNQ3ym_GHvONnrBypg/edit#heading=h.n1ipt86esc34 + if (TossCoin()) { + DVLOG(1) << "Step 6: Special Block"; + items = GenerateSpecialBlock(info.suggested_publisher_ids); + } else { + DVLOG(1) << "Step 6: None (approximately half the time)"; + } + } else { + NOTREACHED(); + } + + // If we couldn't generate a normal block, break. + if (iteration_type == 0 && items.empty()) { + break; + } + + DVLOG(1) << "Adding " << items.size() << " new articles (iteration type is " + << iteration_type << "). Currently have " << feed->items.size() + << " articles"; + add_items(items); + ++iteration; + } + + return feed; +} + FeedV2Builder::FeedV2Builder( PublishersController& publishers_controller, ChannelsController& channels_controller, @@ -832,42 +1041,44 @@ void FeedV2Builder::AddListener( } void FeedV2Builder::BuildFollowingFeed(BuildFeedCallback callback) { + FeedItems raw_feed_items; + base::ranges::transform(raw_feed_items_, std::back_inserter(raw_feed_items), + [](const auto& item) { return item->Clone(); }); GenerateFeed( {.signals = true}, mojom::FeedV2Type::NewFollowing(mojom::FeedV2FollowingType::New()), - base::BindOnce( - [](FeedV2Builder* builder) { - return builder->GenerateBasicFeed( - builder->raw_feed_items_, base::BindRepeating(&PickRoulette), - base::BindRepeating(&PickRoulette)); - }, - base::Unretained(this)), + base::BindOnce([](FeedGenerationInfo info) { + return GenerateBasicFeed(std::move(info), + base::BindRepeating(&PickRoulette), + base::BindRepeating(&PickRoulette)); + }), std::move(callback)); } void FeedV2Builder::BuildChannelFeed(const std::string& channel, BuildFeedCallback callback) { + FeedItems raw_feed_items; + base::ranges::transform(raw_feed_items_, std::back_inserter(raw_feed_items), + [](const auto& item) { return item->Clone(); }); GenerateFeed( {.signals = true}, mojom::FeedV2Type::NewChannel(mojom::FeedV2ChannelType::New(channel)), base::BindOnce( - [](FeedV2Builder* builder, const std::string& channel) { - auto& publishers = - builder->publishers_controller_->GetLastPublishers(); - auto& locale = builder->publishers_controller_->GetLastLocale(); - + [](std::string channel, FeedGenerationInfo info) { FeedItems feed_items; - for (const auto& item : builder->raw_feed_items_) { + for (const auto& item : info.feed_items) { if (!item->is_article()) { continue; } auto publisher_it = - publishers.find(item->get_article()->data->publisher_id); - if (publisher_it == publishers.end()) { + info.publishers.find(item->get_article()->data->publisher_id); + if (publisher_it == info.publishers.end()) { continue; } + const auto& locale = info.locale; + auto locale_info_it = base::ranges::find_if(publisher_it->second->locales, [&locale](const auto& locale_info) { @@ -884,11 +1095,13 @@ void FeedV2Builder::BuildChannelFeed(const std::string& channel, feed_items.push_back(item->Clone()); } - return builder->GenerateBasicFeed( - feed_items, base::BindRepeating(&PickRoulette), - base::BindRepeating(&PickRoulette)); + info.feed_items = std::move(feed_items); + + return GenerateBasicFeed(std::move(info), + base::BindRepeating(&PickRoulette), + base::BindRepeating(&PickRoulette)); }, - base::Unretained(this), channel), + channel), std::move(callback)); } @@ -899,10 +1112,10 @@ void FeedV2Builder::BuildPublisherFeed(const std::string& publisher_id, mojom::FeedV2Type::NewPublisher( mojom::FeedV2PublisherType::New(publisher_id)), base::BindOnce( - [](FeedV2Builder* builder, const std::string& publisher_id) { + [](const std::string& publisher_id, FeedGenerationInfo info) { FeedItems items; - for (const auto& item : builder->raw_feed_items_) { + for (const auto& item : info.feed_items) { if (!item->is_article()) { continue; } @@ -919,20 +1132,20 @@ void FeedV2Builder::BuildPublisherFeed(const std::string& publisher_id, b->get_article()->data->publish_time; }); - return builder->GenerateBasicFeed( - items, base::BindRepeating(&PickFirstIndex), - base::BindRepeating(&PickFirstIndex)); + info.feed_items = std::move(items); + + return GenerateBasicFeed(std::move(info), + base::BindRepeating(&PickFirstIndex), + base::BindRepeating(&PickFirstIndex)); }, - base::Unretained(this), publisher_id), + publisher_id), std::move(callback)); } void FeedV2Builder::BuildAllFeed(BuildFeedCallback callback) { - GenerateFeed( - {.signals = true, .suggested_publishers = true}, - mojom::FeedV2Type::NewAll(mojom::FeedV2AllType::New()), - base::BindOnce(&FeedV2Builder::GenerateAllFeed, base::Unretained(this)), - std::move(callback)); + GenerateFeed({.signals = true, .suggested_publishers = true}, + mojom::FeedV2Type::NewAll(mojom::FeedV2AllType::New()), + base::BindOnce(&GenerateAllFeed), std::move(callback)); } void FeedV2Builder::EnsureFeedIsUpdating() { @@ -948,8 +1161,7 @@ void FeedV2Builder::GetSignals(GetSignalsCallback callback) { base::BindOnce( [](base::WeakPtr builder, GetSignalsCallback callback) { - if (builder) { - std::move(callback).Run({}); + if (!builder) { return; } base::flat_map signals; @@ -978,7 +1190,7 @@ void FeedV2Builder::OnPublishersUpdated( } void FeedV2Builder::UpdateData(UpdateSettings settings, - base::OnceCallback callback) { + UpdateCallback callback) { if (current_update_) { if (current_update_->IsSufficient(settings)) { current_update_->callbacks.push_back(std::move(callback)); @@ -992,24 +1204,31 @@ void FeedV2Builder::UpdateData(UpdateSettings settings, return; } - if (settings.signals) { + current_update_.emplace(std::move(settings), std::move(callback)); + + PrepareAndFetch(); +} + +void FeedV2Builder::PrepareAndFetch() { + DVLOG(1) << __FUNCTION__; + CHECK(current_update_); + + if (current_update_->settings.signals) { signals_.clear(); } - if (settings.feed) { + if (current_update_->settings.feed) { raw_feed_items_.clear(); } - if (settings.suggested_publishers) { + if (current_update_->settings.suggested_publishers) { suggested_publisher_ids_.clear(); } - if (settings.topics) { + if (current_update_->settings.topics) { topics_.clear(); } - current_update_.emplace(std::move(settings), std::move(callback)); - FetchFeed(); } @@ -1122,240 +1341,99 @@ void FeedV2Builder::NotifyUpdateCompleted() { std::tie(hash_, subscribed_count_) = GetFeedHashAndSubscribedCount(channels, publishers, feed_etags_); - // Fire all the pending callbacks. for (auto& callback : current_update_->callbacks) { std::move(callback).Run(); } - // If we have a |next_update_| then request an update with that data. - current_update_ = std::move(next_update_); - next_update_ = absl::nullopt; - // Notify listeners of updated hash. for (const auto& listener : listeners_) { listener->OnUpdateAvailable(hash_); } + // Move |next_update_| into |current_update_|, and kick it off (if set). + current_update_ = std::move(next_update_); + next_update_ = std::nullopt; + if (current_update_) { - UpdateData(current_update_->settings); + PrepareAndFetch(); } } -void FeedV2Builder::GenerateFeed( - UpdateSettings settings, - mojom::FeedV2TypePtr type, - base::OnceCallback build_feed, - BuildFeedCallback callback) { +void FeedV2Builder::GenerateFeed(UpdateSettings settings, + mojom::FeedV2TypePtr type, + FeedGenerator generator, + BuildFeedCallback callback) { + DCHECK_CURRENTLY_ON(content::BrowserThread::UI); UpdateData( std::move(settings), base::BindOnce( - [](base::WeakPtr builder, mojom::FeedV2TypePtr type, - base::OnceCallback build_feed, + [](const base::WeakPtr builder, + mojom::FeedV2TypePtr type, FeedGenerator generate, BuildFeedCallback callback) { if (!builder) { std::move(callback).Run(mojom::FeedV2::New()); return; } - auto feed = std::move(build_feed).Run(); - feed->construct_time = base::Time::Now(); - feed->type = std::move(type); - feed->source_hash = builder->hash_; - - if (feed->items.empty()) { - // If we have no subscribed items and we've loaded the list of - // publishers (which we might not have, if we're offline) then - // we're not subscribed to any feeds. - if (builder->subscribed_count_ == 0 && - !builder->publishers_controller_->GetLastPublishers() - .empty()) { - feed->error = mojom::FeedV2Error::NoFeeds; - // If we don't have any raw feed items (and we're subscribed to - // some feeds) then fetching must have failed. - } else if (builder->raw_feed_items_.size() == 0) { - feed->error = mojom::FeedV2Error::ConnectionError; - // Otherwise, this feed must have no articles. - } else { - feed->error = mojom::FeedV2Error::NoArticles; + const auto& publishers = + builder->publishers_controller_->GetLastPublishers(); + const auto& locale = + builder->publishers_controller_->GetLastLocale(); + std::vector channels; + for (const auto& [channel_id, channel] : + builder->channels_controller_->GetChannelsFromPublishers( + publishers, &*builder->prefs_)) { + if (base::Contains(channel->subscribed_locales, locale)) { + channels.push_back(channel_id); } } - std::move(callback).Run(feed->Clone()); + FeedGenerationInfo info( + locale, builder->raw_feed_items_, publishers, + std::move(channels), builder->signals_, + builder->suggested_publisher_ids_, builder->topics_); + + // We post this to another thread, because the generation process + // can be quite slow. It's safe because all data |generator| + // requires is copied into the lambda. + base::ThreadPool::PostTaskAndReplyWithResult( + FROM_HERE, base::BindOnce(std::move(generate), std::move(info)), + base::BindOnce( + [](size_t raw_feed_items_size, size_t subscribed_count, + bool has_publishers, std::string hash, + mojom::FeedV2TypePtr type, BuildFeedCallback callback, + mojom::FeedV2Ptr feed) { + DCHECK_CURRENTLY_ON(content::BrowserThread::UI); + feed->construct_time = base::Time::Now(); + feed->type = std::move(type); + feed->source_hash = hash; + + if (feed->items.empty()) { + // If we have no subscribed items and we've loaded + // the list of publishers (which we might not have, + // if we're offline) then we're not subscribed to + // any feeds. + if (subscribed_count == 0 && has_publishers) { + feed->error = mojom::FeedV2Error::NoFeeds; + // If we don't have any raw feed items (and we're + // subscribed to some feeds) then fetching must + // have failed. + } else if (raw_feed_items_size == 0) { + feed->error = mojom::FeedV2Error::ConnectionError; + // Otherwise, this feed must have no articles. + } else { + feed->error = mojom::FeedV2Error::NoArticles; + } + } + + std::move(callback).Run(std::move(feed)); + }, + builder->raw_feed_items_.size(), builder->subscribed_count_, + !publishers.empty(), builder->hash_, std::move(type), + std::move(callback))); }, - weak_ptr_factory_.GetWeakPtr(), std::move(type), - std::move(build_feed), std::move(callback))); -} - -mojom::FeedV2Ptr FeedV2Builder::GenerateBasicFeed(const FeedItems& feed_items, - PickArticles pick_hero, - PickArticles pick_article) { - DVLOG(1) << __FUNCTION__; - - auto suggested_publisher_ids = suggested_publisher_ids_; - auto articles = - GetArticleInfos(publishers_controller_->GetLastLocale(), feed_items, - publishers_controller_->GetLastPublishers(), signals_); - - auto feed = mojom::FeedV2::New(); - - constexpr size_t kIterationsPerAd = 2; - size_t blocks = 0; - while (!articles.empty()) { - auto items = GenerateBlock(articles, pick_hero, pick_article, - /*inline_discovery_ratio=*/0); - if (items.empty()) { - break; - } - - // After the first block, every second block should be an ad. - if (blocks % kIterationsPerAd == 0 && blocks != 0) { - std::ranges::move(GenerateSpecialBlock(suggested_publisher_ids), - std::back_inserter(items)); - } - - std::ranges::move(items, std::back_inserter(feed->items)); - blocks++; - } - - // Insert an ad as the second item. - if (feed->items.size() > 1) { - auto ad = GenerateAd(); - feed->items.insert(feed->items.begin() + 1, - std::make_move_iterator(ad.begin()), - std::make_move_iterator(ad.end())); - } - - return feed; -} - -mojom::FeedV2Ptr FeedV2Builder::GenerateAllFeed() { - DVLOG(1) << __FUNCTION__; - const auto& publishers = publishers_controller_->GetLastPublishers(); - const auto& locale = publishers_controller_->GetLastLocale(); - - // Get a list of the subscribed channels - we'll use these when determining - // what channel cards to show. - Channels channels = - channels_controller_->GetChannelsFromPublishers(publishers, &*prefs_); - - std::vector subscribed_channels; - for (const auto& [id, channel] : channels) { - if (base::Contains(channel->subscribed_locales, locale)) { - subscribed_channels.push_back(id); - DVLOG(1) << "Subscribed to channel: " << id; - } - } - - // Make a copy of these - we're going to edit the copy to prevent duplicates. - auto suggested_publisher_ids = suggested_publisher_ids_; - auto articles = - GetArticleInfos(locale, raw_feed_items_, publishers, signals_); - auto feed = mojom::FeedV2::New(); - - base::span topics = base::make_span(topics_); - - auto add_items = [&feed](std::vector& items) { - base::ranges::move(items, std::back_inserter(feed->items)); - }; - - std::vector eligible_content_groups; - for (const auto& channel_id : subscribed_channels) { - eligible_content_groups.push_back(std::make_pair(channel_id, true)); - } - for (const auto& [publisher_id, publisher] : publishers) { - if (publisher->user_enabled_status == mojom::UserEnabled::ENABLED || - publisher->type == mojom::PublisherType::DIRECT_SOURCE) { - eligible_content_groups.push_back(std::make_pair(publisher_id, false)); - DVLOG(1) << "Subscribed to publisher: " << publisher->publisher_name; - } - } - - // If we aren't subscribed to anything, or we failed to fetch any articles - // from the internet, don't try and generate a feed. - if (eligible_content_groups.size() == 0 || raw_feed_items_.size() == 0) { - return feed; - } - - // Step 1: Generate a block - // https://docs.google.com/document/d/1bSVHunwmcHwyQTpa3ab4KRbGbgNQ3ym_GHvONnrBypg/edit#heading=h.rkq699fwps0 - std::vector initial_block = - GenerateBlockFromContentGroups(articles, locale, publishers, - eligible_content_groups); - DVLOG(1) << "Step 1: Standard Block (" << initial_block.size() - << " articles)"; - add_items(initial_block); - - // Step 2: We always add an advertisment after the first block. - // https://docs.google.com/document/d/1bSVHunwmcHwyQTpa3ab4KRbGbgNQ3ym_GHvONnrBypg/edit#heading=h.82154jsxm16 - auto advert = GenerateAd(); - DVLOG(1) << "Step 2: Advertisement"; - add_items(advert); - - // Step 3: Generate a top news block - // https://docs.google.com/document/d/1bSVHunwmcHwyQTpa3ab4KRbGbgNQ3ym_GHvONnrBypg/edit#heading=h.7z05nb4b269d - // This block is a bit special - we take the top articles from the top few - // topics and display them in a cluster. If there are no topics, we try and do - // the same thing, but with the Top News channel. - auto top_news_block = GenerateTopTopicsBlock(publishers, topics); - if (top_news_block.empty()) { - top_news_block = - GenerateChannelBlock(locale, publishers, kTopNewsChannel, articles); - } - DVLOG(1) << "Step 3: Top News Block"; - add_items(top_news_block); - - // Repeat step 4 - 6 until we don't have any more articles to add to the feed. - constexpr uint8_t kIterationTypes = 3; - uint32_t iteration = 0; - while (true) { - std::vector items; - - auto iteration_type = iteration % kIterationTypes; - - // Step 4: Block Generation - // https://docs.google.com/document/d/1bSVHunwmcHwyQTpa3ab4KRbGbgNQ3ym_GHvONnrBypg/edit#heading=h.os2ze8cesd8v - if (iteration_type == 0) { - DVLOG(1) << "Step 4: Standard Block"; - items = GenerateBlockFromContentGroups(articles, locale, publishers, - eligible_content_groups); - } else if (iteration_type == 1) { - // Step 5: Block or Cluster Generation - // https://docs.google.com/document/d/1bSVHunwmcHwyQTpa3ab4KRbGbgNQ3ym_GHvONnrBypg/edit#heading=h.tpvsjkq0lzmy - // Half the time, a normal block - if (TossCoin()) { - DVLOG(1) << "Step 5: Standard Block"; - items = GenerateBlockFromContentGroups(articles, locale, publishers, - eligible_content_groups); - } else { - items = GenerateClusterBlock(locale, publishers, subscribed_channels, - topics, articles); - } - } else if (iteration_type == 2) { - // Step 6: Optional special card - // https://docs.google.com/document/d/1bSVHunwmcHwyQTpa3ab4KRbGbgNQ3ym_GHvONnrBypg/edit#heading=h.n1ipt86esc34 - if (TossCoin()) { - DVLOG(1) << "Step 6: Special Block"; - items = GenerateSpecialBlock(suggested_publisher_ids); - } else { - DVLOG(1) << "Step 6: None (approximately half the time)"; - } - } else { - NOTREACHED(); - } - - // If we couldn't generate a normal block, break. - if (iteration_type == 0 && items.empty()) { - break; - } - - DVLOG(1) << "Adding " << items.size() << " new articles (iteration type is " - << iteration_type << "). Currently have " << feed->items.size() - << " articles"; - add_items(items); - ++iteration; - } - - return feed; + weak_ptr_factory_.GetWeakPtr(), std::move(type), std::move(generator), + std::move(callback))); } } // namespace brave_news diff --git a/components/brave_news/browser/feed_v2_builder.h b/components/brave_news/browser/feed_v2_builder.h index 209322dc36d6..df0595d9a5ef 100644 --- a/components/brave_news/browser/feed_v2_builder.h +++ b/components/brave_news/browser/feed_v2_builder.h @@ -89,7 +89,14 @@ class FeedV2Builder : public PublishersController::Observer { void OnPublishersUpdated(PublishersController* controller) override; private: - using UpdateCallback = base::OnceCallback; + struct FeedGenerationInfo; + + // FeedGenerator's will be called on a different thread. The data in + // |FeedGenerationInfo| is a copy and can be safely modified. + using FeedGenerator = + base::OnceCallback; + + using UpdateCallback = base::OnceClosure; struct UpdateSettings { bool signals = false; bool suggested_publishers = false; @@ -134,9 +141,14 @@ class FeedV2Builder : public PublishersController::Observer { UpdateCallback callback); }; - void UpdateData(UpdateSettings settings, - UpdateCallback callback = base::DoNothing()); + static mojom::FeedV2Ptr GenerateBasicFeed(FeedGenerationInfo info, + PickArticles pick_hero, + PickArticles pick_article); + static mojom::FeedV2Ptr GenerateAllFeed(FeedGenerationInfo info); + void UpdateData(UpdateSettings settings, UpdateCallback callback); + + void PrepareAndFetch(); void FetchFeed(); void OnFetchedFeed(FeedItems items, ETags etags); @@ -154,14 +166,9 @@ class FeedV2Builder : public PublishersController::Observer { void GenerateFeed(UpdateSettings settings, mojom::FeedV2TypePtr type, - base::OnceCallback build_feed, + FeedGenerator generator, BuildFeedCallback callback); - mojom::FeedV2Ptr GenerateBasicFeed(const FeedItems& items, - PickArticles pick_hero, - PickArticles pick_article); - mojom::FeedV2Ptr GenerateAllFeed(); - raw_ref publishers_controller_; raw_ref channels_controller_; raw_ref suggestions_controller_;