Discharge7443/brave-core
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #15779 from brave/news-desktop-v2-enable

Browse Source 
Enable news v2 by default for Desktop
This commit is contained in:
Pete Miller
2022-11-08 12:30:17 -08:00
committed by GitHub
parent 15a2aef170 d00738721c
commit c715ec7916
6 changed files with 109 additions and 31 deletions
Download Patch File Download Diff File Expand all files Collapse all files
components/brave_today/browser/channels_controller.cc
+18 -11
View File
@@ -34,6 +34,20 @@ bool IsChannelSubscribedInLocale(const base::Value::Dict& subscriptions,
}
} // namespace
// static
void ChannelsController::SetChannelSubscribedPref(PrefService* prefs,
const std::string& locale,
const std::string& channel_id,
bool subscribed) {
DictionaryPrefUpdate update(prefs, prefs::kBraveNewsChannels);
auto* locale_dict = update->GetDict().EnsureDict(locale);
if (!subscribed) {
locale_dict->Remove(channel_id);
} else {
locale_dict->Set(channel_id, true);
}
}
ChannelsController::ChannelsController(
PrefService* prefs,
PublishersController* publishers_controller)
@@ -111,18 +125,11 @@ mojom::ChannelPtr ChannelsController::SetChannelSubscribed(
const std::string& locale,
const std::string& channel_id,
bool subscribed) {
// Note: DictionaryPrefUpdate is nested here so the update happens before
// we call |GetChannelLocales|.
{
DictionaryPrefUpdate update(prefs_, prefs::kBraveNewsChannels);
auto* locale_dict = update->GetDict().EnsureDict(locale);
if (!subscribed) {
locale_dict->Remove(channel_id);
} else {
locale_dict->Set(channel_id, true);
}
}
// Persist the pref
ChannelsController::SetChannelSubscribedPref(prefs_, locale, channel_id,
subscribed);
// Provide an updated entity
auto result = mojom::Channel::New();
result->channel_name = channel_id;
result->subscribed_locales = GetChannelLocales(channel_id);
components/brave_today/browser/channels_controller.h
+5
View File
@@ -47,6 +47,11 @@ class ChannelsController {
const std::string& channel_id);
private:
FRIEND_TEST_ALL_PREFIXES(BraveNewsFeedBuildingTest, BuildFeedV2);
static void SetChannelSubscribedPref(PrefService* prefs,
const std::string& locale,
const std::string& channel_id,
bool subscribed);
raw_ptr<PrefService> prefs_;
raw_ptr<PublishersController> publishers_controller_;
};
components/brave_today/browser/feed_building.cc
+10 -8
View File
@@ -267,14 +267,16 @@ bool ShouldDisplayFeedItem(const mojom::FeedItemPtr& feed_item,
// it belongs to are subscribed to.
for (const auto& locale_info : publisher->locales) {
for (const auto& channel_id : locale_info->channels) {
const auto& channel = channels.at(channel_id);
if (base::Contains(channel->subscribed_locales,
locale_info->locale)) {
VLOG(2) << "Showing article because publisher "
<< data->publisher_id << ": " << publisher->publisher_name
<< " is in channel " << locale_info->locale << "."
<< channel_id << " which is subscribed to.";
return true;
if (channels.contains(channel_id)) {
const auto& channel = channels.at(channel_id);
if (base::Contains(channel->subscribed_locales,
locale_info->locale)) {
VLOG(2) << "Showing article because publisher "
<< data->publisher_id << ": " << publisher->publisher_name
<< " is in channel " << locale_info->locale << "."
<< channel_id << " which is subscribed to.";
return true;
}
}
}
}
components/brave_today/browser/feed_building_unittest.cc
+55 -7
View File
@@ -81,7 +81,7 @@ std::string GetFeedJson() {
"score": 13.96799592432192
},
{
"category": "Technology",
"category": "Top News",
"publish_time": "2021-09-01 07:01:28",
"url": "https://www.digitaltrends.com/computing/logi-bolt-secure-wireless-connectivity/",
"title": "Expecting Second Logitech built Bolt to make wireless mice and keyboards work better",
@@ -92,7 +92,7 @@ std::string GetFeedJson() {
"creative_instance_id": "",
"url_hash": "523b9f2091474c2a082c06ec17965f8c2392f871917407228bbeb51d8a55d6be",
"padded_img": "https://pcdn.brave.com/brave-today/cache/052e832456e00a3cee51c68eee206fe71c32cba35d5e53dee2777dd132e01364.jpg.pad",
"score": 13.91160989810695
"score": 13.97160989810695
}
]
)";
@@ -114,13 +114,13 @@ std::vector<mojom::LocaleInfoPtr> CreateLocales(
void PopulatePublishers(Publishers* publisher_list) {
auto publisher1 = mojom::Publisher::New(
"111", mojom::PublisherType::COMBINED_SOURCE, "First Publisher",
"Top News", true, CreateLocales({"en_US"}, {"Top News"}),
"Top News", true, CreateLocales({"en_US"}, {"Top News", "Top Sources"}),
GURL("https://www.example.com"), absl::nullopt, absl::nullopt,
absl::nullopt, GURL("https://first-publisher.com/feed.xml"),
mojom::UserEnabled::NOT_MODIFIED);
auto publisher2 = mojom::Publisher::New(
"222", mojom::PublisherType::COMBINED_SOURCE, "Second Publisher",
"Top News", true, CreateLocales({"en_US"}, {"Top News"}),
"Top News", true, CreateLocales({"en_US"}, {"Top News", "Top Sources"}),
GURL("https://www.example.com"), absl::nullopt, absl::nullopt,
absl::nullopt, GURL("https://second-publisher.com/feed.xml"),
mojom::UserEnabled::NOT_MODIFIED);
@@ -153,7 +153,11 @@ class BraveNewsFeedBuildingTest : public testing::Test {
TestingProfile profile_;
};
TEST_F(BraveNewsFeedBuildingTest, BuildFeed) {
TEST_F(BraveNewsFeedBuildingTest, BuildFeedV1) {
// Use v1 feed strategy
base::test::ScopedFeatureList features;
features.InitAndDisableFeature(brave_today::features::kBraveNewsV2Feature);
Publishers publisher_list;
PopulatePublishers(&publisher_list);
@@ -183,10 +187,50 @@ TEST_F(BraveNewsFeedBuildingTest, BuildFeed) {
"live-transfer-deadline-day-will-real-madrid-land-psg-star-mbappe");
ASSERT_EQ(feed.pages[0]->items[1]->items.size(), 1u);
ASSERT_EQ(feed.pages[0]->items[1]->items[0]->get_article()->data->url,
"https://www.digitaltrends.com/computing/"
"logi-bolt-secure-wireless-connectivity/");
"https://www.example.com/an-article/");
ASSERT_EQ(feed.pages[0]->items[2]->items.size(), 1u);
ASSERT_EQ(feed.pages[0]->items[2]->items[0]->get_article()->data->url,
"https://www.digitaltrends.com/computing/"
"logi-bolt-secure-wireless-connectivity/");
}
TEST_F(BraveNewsFeedBuildingTest, BuildFeedV2) {
// Use v2 feed strategy by subscribing to a channel
base::test::ScopedFeatureList features;
features.InitAndEnableFeature(brave_today::features::kBraveNewsV2Feature);
ChannelsController::SetChannelSubscribedPref(profile_.GetPrefs(), "en_US",
"Top Sources", true);
Publishers publisher_list;
PopulatePublishers(&publisher_list);
std::unordered_set<std::string> history_hosts = {"www.espn.com"};
std::vector<mojom::FeedItemPtr> feed_items;
ParseFeedItems(GetFeedJson(), &feed_items);
mojom::Feed feed;
ASSERT_TRUE(BuildFeed(feed_items, history_hosts, &publisher_list, &feed,
profile_.GetPrefs()));
ASSERT_EQ(feed.pages.size(), 1u);
// Validate featured article is top news
ASSERT_TRUE(feed.featured_item->is_article());
ASSERT_EQ(feed.featured_item->get_article()->data->url.spec(),
"https://www.digitaltrends.com/computing/"
"logi-bolt-secure-wireless-connectivity/");
// Validate sorted by score descending
ASSERT_GE(feed.pages[0]->items.size(), 2u);
// Because we cannot access a flat list, then select the items from each card
// (some cards have 1 item, some have 2, etc). If the page_content_order
// changes, then also change here which items we access in which order.
ASSERT_EQ(feed.pages[0]->items[0]->items.size(), 1u);
ASSERT_EQ(feed.pages[0]->items[0]->items[0]->get_article()->data->url,
"https://www.espn.com/soccer/blog-transfer-talk/story/4465789/"
"live-transfer-deadline-day-will-real-madrid-land-psg-star-mbappe");
ASSERT_EQ(feed.pages[0]->items[1]->items.size(), 1u);
ASSERT_EQ(feed.pages[0]->items[1]->items[0]->get_article()->data->url,
"https://www.example.com/an-article/");
}
@@ -221,6 +265,10 @@ TEST_F(BraveNewsFeedBuildingTest, DirectFeedsShouldAlwaysBeDisplayed) {
}
TEST_F(BraveNewsFeedBuildingTest, RemovesDefaultOffItems) {
// Use v1 feed strategy
base::test::ScopedFeatureList features;
features.InitAndDisableFeature(brave_today::features::kBraveNewsV2Feature);
Publishers publisher_list;
PopulatePublishers(&publisher_list);
std::unordered_set<std::string> history_hosts = {};
components/brave_today/browser/urls_unittest.cc
+11 -3
View File
@@ -15,12 +15,20 @@
class BraveNewsUrlsTest : public testing::Test {};
TEST_F(BraveNewsUrlsTest, BraveNewsV2IsDisabled) {
TEST_F(BraveNewsUrlsTest, BraveNewsV2FeatureFlag) {
#if BUILDFLAG(IS_ANDROID)
EXPECT_FALSE(
base::FeatureList::IsEnabled(brave_today::features::kBraveNewsV2Feature));
#else
EXPECT_TRUE(
base::FeatureList::IsEnabled(brave_today::features::kBraveNewsV2Feature));
#endif
}
TEST_F(BraveNewsUrlsTest, BraveNewsUsesV1ByDefault) {
TEST_F(BraveNewsUrlsTest, BraveNewsV1UsesLocalUrl) {
base::test::ScopedFeatureList features;
features.InitAndDisableFeature(brave_today::features::kBraveNewsV2Feature);
{
brave_l10n::test::ScopedDefaultLocale scoped_default_locale{"en_US"};
std::string region = brave_today::GetRegionUrlPart();
@@ -44,7 +52,7 @@ TEST_F(BraveNewsUrlsTest, BraveNewsUsesV1ByDefault) {
}
}
TEST_F(BraveNewsUrlsTest, BraveNewsV2FlagUsesGlobalFeeds) {
TEST_F(BraveNewsUrlsTest, BraveNewsUsesGlobalFeedsWithV2) {
base::test::ScopedFeatureList features;
features.InitAndEnableFeature(brave_today::features::kBraveNewsV2Feature);
components/brave_today/common/features.cc
+10 -2
View File
@@ -12,8 +12,16 @@ namespace features {
const base::Feature kBraveNewsFeature{"BraveNews",
base::FEATURE_ENABLED_BY_DEFAULT};
const base::Feature kBraveNewsV2Feature{"BraveNewsV2",
base::FEATURE_DISABLED_BY_DEFAULT};
const base::Feature kBraveNewsV2Feature {
"BraveNewsV2",
#if BUILDFLAG(IS_ANDROID)
base::FEATURE_DISABLED_BY_DEFAULT
#else
base::FEATURE_ENABLED_BY_DEFAULT
#endif
};
const base::Feature kBraveNewsCardPeekFeature{"BraveNewsCardPeek",
base::FEATURE_ENABLED_BY_DEFAULT};
const base::Feature kBraveNewsSubscribeButtonFeature{