Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Uplift 1.46.x] Brave News bug fixes #16028

Merged
merged 4 commits into from
Nov 21, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion browser/brave_news/brave_news_tab_helper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,8 @@ BraveNewsTabHelper::GetAvailableFeeds() {
bool BraveNewsTabHelper::IsSubscribed(const FeedDetails& feed_details) {
auto* publisher = controller_->publisher_controller()->GetPublisherForFeed(
feed_details.feed_url);
return brave_news::IsPublisherEnabled(publisher);
return publisher && publisher->user_enabled_status ==
brave_news::mojom::UserEnabled::ENABLED;
}

bool BraveNewsTabHelper::IsSubscribed() {
Expand Down
4 changes: 4 additions & 0 deletions chromium_src/ui/views/controls/button/md_text_button.cc
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,10 @@ void MdTextButton::UpdateColorsForBrave() {
SkColor bg_color = GetBgColorOverride().value_or(
style.background_color.value_or(SK_ColorTRANSPARENT));
SkColor stroke_color = style.border_color.value_or(SK_ColorTRANSPARENT);

// SubPixelRendering doesn't work if we have any background opacity.
SetTextSubpixelRenderingEnabled(opacity == 1 &&
SkColorGetA(bg_color) == 0xFF);
SetBackground(
CreateBackgroundFromPainter(Painter::CreateRoundRectWith1PxBorderPainter(
AddOpacity(bg_color, opacity), AddOpacity(stroke_color, opacity),
Expand Down
24 changes: 15 additions & 9 deletions components/brave_today/browser/brave_news_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,6 @@ namespace {
constexpr uint32_t kDesiredFaviconSizePixels = 48;
} // namespace

bool IsPublisherEnabled(const mojom::Publisher* publisher) {
if (!publisher)
return false;
return (publisher->is_enabled &&
publisher->user_enabled_status != mojom::UserEnabled::DISABLED) ||
publisher->user_enabled_status == mojom::UserEnabled::ENABLED;
}

// static
void BraveNewsController::RegisterProfilePrefs(PrefRegistrySimple* registry) {
registry->RegisterBooleanPref(prefs::kShouldShowToolbarButton, true);
Expand Down Expand Up @@ -615,11 +607,25 @@ void BraveNewsController::HandleSubscriptionsChanged() {
void BraveNewsController::MaybeInitPrefs() {
if (GetIsEnabled() && base::FeatureList::IsEnabled(
brave_today::features::kBraveNewsV2Feature)) {
// We had a bug where you could be subscribed to a channel in the empty
// locale in earlier versions of Brave News. If so, we should remove it.
// After this has been out for a bit we can remove it.
// https://github.com/brave/brave-browser/issues/26596
if (prefs_->GetDict(prefs::kBraveNewsChannels).contains("")) {
ScopedDictPrefUpdate update(prefs_, prefs::kBraveNewsChannels);
update->Remove("");
}

const auto& channels = prefs_->GetDict(prefs::kBraveNewsChannels);
if (channels.empty() && GetIsEnabled()) {
if (channels.empty()) {
publishers_controller_.GetLocale(base::BindOnce(
[](ChannelsController* channels_controller,
const std::string& locale) {
// This could happen, if we're offline, or the API is down at the
// moment.
if (locale.empty()) {
return;
}
channels_controller->SetChannelSubscribed(locale,
kTopSourcesChannel, true);
},
Expand Down
2 changes: 0 additions & 2 deletions components/brave_today/browser/brave_news_controller.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,6 @@ class HistoryService;

namespace brave_news {

bool IsPublisherEnabled(const mojom::Publisher* publisher);

// Browser-side handler for Brave News mojom API, 1 per profile
// Orchestrates FeedController and PublishersController for data, as well as
// owning prefs data.
Expand Down
2 changes: 2 additions & 0 deletions components/brave_today/browser/channels_controller.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ class ChannelsController {

private:
FRIEND_TEST_ALL_PREFIXES(BraveNewsFeedBuildingTest, BuildFeedV2);
FRIEND_TEST_ALL_PREFIXES(BraveNewsFeedBuildingTest,
DuplicateItemsAreNotIncluded);
static void SetChannelSubscribedPref(PrefService* prefs,
const std::string& locale,
const std::string& channel_id,
Expand Down
35 changes: 13 additions & 22 deletions components/brave_today/browser/direct_feed_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
#include "base/callback.h"
#include "base/containers/flat_set.h"
#include "base/guid.h"
#include "base/i18n/icu_string_conversions.h"
#include "base/location.h"
#include "base/logging.h"
#include "base/strings/string_util.h"
Expand Down Expand Up @@ -86,33 +85,26 @@ mojom::ArticlePtr RustFeedItemToArticle(const FeedItem& rust_feed_item) {

using ParseFeedCallback = base::OnceCallback<void(absl::optional<FeedData>)>;
void ParseFeedDataOffMainThread(const GURL& feed_url,
const std::string& charset,
std::string body_content,
ParseFeedCallback callback) {
base::ThreadPool::PostTaskAndReplyWithResult(
FROM_HERE,
base::BindOnce(
[](const GURL& feed_url, const std::string& charset,
[](const GURL& feed_url,
std::string body_content) -> absl::optional<FeedData> {
std::string result;
// Ensure the body is encoded as UTF-8.
if (!base::ConvertToUtf8AndNormalize(body_content, charset,
&result)) {
LOG(ERROR) << "Failed to convert body content of " << feed_url
<< " to utf-8";
return absl::nullopt;
}

brave_news::FeedData data;
if (!parse_feed_string(::rust::String(result), data)) {
if (!parse_feed_bytes(::rust::Slice<const uint8_t>(
(const uint8_t*)body_content.data(),
body_content.size()),
data)) {
VLOG(1) << feed_url.spec() << " not a valid feed.";
VLOG(2) << "Response body was:";
VLOG(2) << result;
VLOG(2) << body_content;
return absl::nullopt;
}
return data;
},
feed_url, charset, std::move(body_content)),
feed_url, std::move(body_content)),
std::move(callback));
}

Expand Down Expand Up @@ -207,9 +199,9 @@ void DirectFeedController::FindFeeds(
auto* loader = iter->get();
auto response_code = -1;
std::string mime_type;
std::string charset = GetResponseCharset(loader);
GURL final_url(loader->GetFinalURL());
base::flat_map<std::string, std::string> headers;
std::string charset = GetResponseCharset(loader);
if (loader->ResponseInfo()) {
auto headers_list = loader->ResponseInfo()->headers;
mime_type = loader->ResponseInfo()->mime_type;
Expand All @@ -231,10 +223,10 @@ void DirectFeedController::FindFeeds(

// Response is valid, but still might not be a feed
ParseFeedDataOffMainThread(
feed_url, charset, body_content,
feed_url, body_content,
base::BindOnce(
[](const GURL& feed_url, const GURL& final_url,
const std::string& mime_type,
const std::string& charset, const std::string& mime_type,
const std::string& body_content,
DirectFeedController* direct_feed_controller,
mojom::BraveNewsController::FindFeedsCallback callback,
Expand All @@ -255,7 +247,7 @@ void DirectFeedController::FindFeeds(
VLOG(1) << "Had html type";
// Get feed links from doc
auto feed_urls = GetFeedURLsFromHTMLDocument(
body_content, final_url);
charset, body_content, final_url);
auto all_done_handler = base::BindOnce(
[](mojom::BraveNewsController::FindFeedsCallback
callback,
Expand Down Expand Up @@ -298,7 +290,7 @@ void DirectFeedController::FindFeeds(
VLOG(2) << body_content;
std::move(callback).Run(std::move(results));
},
feed_url, final_url, mime_type, body_content,
feed_url, final_url, charset, mime_type, body_content,
base::Unretained(direct_feed_controller),
std::move(callback)));
},
Expand Down Expand Up @@ -442,7 +434,6 @@ void DirectFeedController::OnResponse(
// Parse response data
auto* loader = iter->get();
auto response_code = -1;
std::string charset = GetResponseCharset(loader);
base::flat_map<std::string, std::string> headers;
if (loader->ResponseInfo()) {
auto headers_list = loader->ResponseInfo()->headers;
Expand All @@ -464,7 +455,7 @@ void DirectFeedController::OnResponse(
}

// Response is valid, but still might not be a feed
ParseFeedDataOffMainThread(feed_url, charset, std::move(body_content),
ParseFeedDataOffMainThread(feed_url, std::move(body_content),
base::BindOnce(
[](DownloadFeedCallback callback,
std::unique_ptr<DirectFeedResponse> result,
Expand Down
108 changes: 106 additions & 2 deletions components/brave_today/browser/direct_feed_controller_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
// you can obtain one at http://mozilla.org/MPL/2.0/.

#include <algorithm>
#include <cstdint>
#include <iterator>
#include <memory>
#include <unordered_set>
Expand Down Expand Up @@ -113,8 +114,10 @@ std::string GetFeedJson() {
TEST(BraveNewsDirectFeed, ParseFeed) {
FeedData data;
// If this errors, probably our xml was not valid
bool parse_success =
brave_news::parse_feed_string(::rust::String(GetFeedJson()), data);
auto json = GetFeedJson();
bool parse_success = brave_news::parse_feed_bytes(
::rust::Slice<const uint8_t>((const uint8_t*)json.data(), json.size()),
data);

// String was parsed to data?
ASSERT_TRUE(parse_success);
Expand Down Expand Up @@ -145,6 +148,107 @@ TEST(BraveNewsDirectFeed, ParseFeed) {
"c5f85f34aa685221604f7e434415ca82");
}

TEST(BraveNewsDirectFeed, ParseWindows1251Feed) {
FeedData data;
std::vector<uint8_t> windows_1251_feed{
60, 63, 120, 109, 108, 32, 118, 101, 114, 115, 105, 111, 110, 61, 34,
49, 46, 48, 34, 32, 101, 110, 99, 111, 100, 105, 110, 103, 61, 34,
119, 105, 110, 100, 111, 119, 115, 45, 49, 50, 53, 49, 34, 32, 63,
62, 10, 60, 114, 115, 115, 32, 118, 101, 114, 115, 105, 111, 110, 61,
34, 50, 46, 48, 34, 62, 10, 10, 60, 99, 104, 97, 110, 110, 101,
108, 62, 10, 32, 32, 60, 116, 105, 116, 108, 101, 62, 119, 105, 110,
100, 111, 119, 115, 45, 49, 50, 53, 49, 32, 70, 101, 101, 100, 32,
78, 97, 109, 101, 60, 47, 116, 105, 116, 108, 101, 62, 10, 32, 32,
60, 108, 105, 110, 107, 62, 104, 116, 116, 112, 115, 58, 47, 47, 119,
119, 119, 46, 119, 51, 115, 99, 104, 111, 111, 108, 115, 46, 99, 111,
109, 60, 47, 108, 105, 110, 107, 62, 10, 32, 32, 60, 100, 101, 115,
99, 114, 105, 112, 116, 105, 111, 110, 62, 65, 32, 116, 101, 115, 116,
32, 102, 101, 101, 100, 60, 47, 100, 101, 115, 99, 114, 105, 112, 116,
105, 111, 110, 62, 10, 32, 32, 60, 105, 116, 101, 109, 62, 10, 32,
32, 32, 32, 60, 116, 105, 116, 108, 101, 62, 85, 107, 114, 97, 105,
110, 105, 97, 110, 60, 47, 116, 105, 116, 108, 101, 62, 10, 32, 32,
32, 32, 60, 108, 105, 110, 107, 62, 104, 116, 116, 112, 115, 58, 47,
47, 119, 119, 119, 46, 101, 120, 97, 109, 112, 108, 101, 46, 99, 111,
109, 47, 111, 110, 101, 47, 119, 105, 110, 100, 111, 119, 115, 45, 49,
50, 53, 49, 60, 47, 108, 105, 110, 107, 62, 10, 32, 32, 32, 32,
60, 112, 117, 98, 68, 97, 116, 101, 62, 84, 104, 117, 44, 32, 49,
55, 32, 78, 111, 118, 32, 50, 48, 50, 50, 32, 49, 54, 58, 49,
48, 58, 48, 51, 32, 69, 83, 84, 60, 47, 112, 117, 98, 68, 97,
116, 101, 62, 10, 32, 32, 32, 32, 60, 100, 101, 115, 99, 114, 105,
112, 116, 105, 111, 110, 62, 207, 207, 206, 44, 32, 224, 240, 242, 232,
235, 229, 240, 179, 255, 44, 32, 225, 238, 186, 239, 240, 232, 239, 224,
241, 232, 58, 32, 227, 235, 224, 226, 224, 32, 207, 229, 237, 242, 224,
227, 238, 237, 243, 60, 47, 100, 101, 115, 99, 114, 105, 112, 116, 105,
111, 110, 62, 10, 32, 32, 60, 47, 105, 116, 101, 109, 62, 10, 60,
47, 99, 104, 97, 110, 110, 101, 108, 62, 10, 10, 60, 47, 114, 115,
115, 62, 10,
};

bool parse_success = brave_news::parse_feed_bytes(
::rust::Slice<const uint8_t>(windows_1251_feed.data(),
windows_1251_feed.size()),
data);

ASSERT_TRUE(parse_success);
ASSERT_EQ(1u, data.items.size());
EXPECT_EQ("windows-1251 Feed Name", (std::string)data.title);
EXPECT_EQ("Ukrainian", (std::string)data.items[0].title);
EXPECT_EQ("ППО, артилерія, боєприпаси: глава Пентагону",
(std::string)data.items[0].description);
}

TEST(BraveNewsDirectFeed, ParseEUCJPFeed) {
FeedData data;
std::vector<uint8_t> euc_jp_feed{
60, 63, 120, 109, 108, 32, 118, 101, 114, 115, 105, 111, 110, 61, 34,
49, 46, 48, 34, 32, 101, 110, 99, 111, 100, 105, 110, 103, 61, 34,
101, 117, 99, 45, 106, 112, 34, 32, 63, 62, 10, 60, 114, 115, 115,
32, 118, 101, 114, 115, 105, 111, 110, 61, 34, 50, 46, 48, 34, 62,
10, 10, 60, 99, 104, 97, 110, 110, 101, 108, 62, 10, 32, 32, 60,
116, 105, 116, 108, 101, 62, 101, 117, 99, 45, 106, 112, 32, 70, 101,
101, 100, 32, 78, 97, 109, 101, 60, 47, 116, 105, 116, 108, 101, 62,
10, 32, 32, 60, 108, 105, 110, 107, 62, 104, 116, 116, 112, 115, 58,
47, 47, 119, 119, 119, 46, 119, 51, 115, 99, 104, 111, 111, 108, 115,
46, 99, 111, 109, 60, 47, 108, 105, 110, 107, 62, 10, 32, 32, 60,
100, 101, 115, 99, 114, 105, 112, 116, 105, 111, 110, 62, 65, 32, 116,
101, 115, 116, 32, 102, 101, 101, 100, 60, 47, 100, 101, 115, 99, 114,
105, 112, 116, 105, 111, 110, 62, 10, 32, 32, 60, 105, 116, 101, 109,
62, 10, 32, 32, 32, 32, 60, 116, 105, 116, 108, 101, 62, 74, 97,
112, 97, 110, 101, 115, 101, 60, 47, 116, 105, 116, 108, 101, 62, 10,
32, 32, 32, 32, 60, 108, 105, 110, 107, 62, 104, 116, 116, 112, 115,
58, 47, 47, 119, 119, 119, 46, 101, 120, 97, 109, 112, 108, 101, 46,
99, 111, 109, 47, 116, 119, 111, 47, 101, 117, 99, 45, 106, 112, 60,
47, 108, 105, 110, 107, 62, 10, 32, 32, 32, 32, 60, 112, 117, 98,
68, 97, 116, 101, 62, 84, 104, 117, 44, 32, 49, 55, 32, 78, 111,
118, 32, 50, 48, 50, 50, 32, 49, 54, 58, 49, 48, 58, 48, 51,
32, 69, 83, 84, 60, 47, 112, 117, 98, 68, 97, 116, 101, 62, 10,
32, 32, 32, 32, 60, 100, 101, 115, 99, 114, 105, 112, 116, 105, 111,
110, 62, 185, 241, 198, 226, 161, 162, 179, 164, 179, 176, 161, 162, 200,
200, 186, 225, 161, 162, 184, 228, 179, 218, 161, 162, 192, 175, 188, 163,
161, 162, 183, 208, 186, 209, 161, 162, 165, 198, 165, 175, 165, 206, 165,
237, 165, 184, 161, 188, 161, 162, 165, 185, 165, 221, 161, 188, 165, 196,
197, 249, 161, 162, 198, 252, 203, 220, 164, 206, 165, 203, 165, 229, 161,
188, 165, 185, 164, 242, 177, 209, 184, 236, 164, 199, 164, 170, 198, 207,
164, 177, 161, 163, 177, 209, 184, 236, 164, 206, 202, 217, 60, 47, 100,
101, 115, 99, 114, 105, 112, 116, 105, 111, 110, 62, 10, 32, 32, 60,
47, 105, 116, 101, 109, 62, 10, 60, 47, 99, 104, 97, 110, 110, 101,
108, 62, 10, 10, 60, 47, 114, 115, 115, 62, 10,
};

bool parse_success = brave_news::parse_feed_bytes(
::rust::Slice<const uint8_t>(euc_jp_feed.data(), euc_jp_feed.size()),
data);

ASSERT_TRUE(parse_success);
ASSERT_EQ(1u, data.items.size());
EXPECT_EQ("euc-jp Feed Name", (std::string)data.title);
EXPECT_EQ("Japanese", (std::string)data.items[0].title);
EXPECT_EQ(
"国内、海外、犯罪、娯楽、政治、経済、テクノロジー、スポーツ等、日本のニュ"
"ースを英語でお届け。英語の勉",
(std::string)data.items[0].description);
}

TEST(BraveNewsDirectFeed, CanAddDirectFeed) {
TestingPrefServiceSimple prefs;
BraveNewsController::RegisterProfilePrefs(prefs.registry());
Expand Down
14 changes: 13 additions & 1 deletion components/brave_today/browser/feed_building.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include <vector>

#include "base/containers/contains.h"
#include "base/containers/flat_set.h"
#include "base/feature_list.h"
#include "base/logging.h"
#include "base/strings/string_number_conversions.h"
Expand All @@ -33,6 +34,7 @@
#include "components/history/core/browser/history_service.h"
#include "components/prefs/pref_service.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
#include "url/gurl.h"

namespace brave_news {

Expand Down Expand Up @@ -295,7 +297,8 @@ bool ShouldDisplayFeedItem(const mojom::FeedItemPtr& feed_item,

// None of the filters match, we can display
VLOG(2) << "None of the filters matched, will display item for publisher "
<< data->publisher_id;
<< data->publisher_id << ": " << publisher->publisher_name << " ["
<< data->title << "]";
return true;
}

Expand All @@ -311,11 +314,20 @@ bool BuildFeed(const std::vector<mojom::FeedItemPtr>& feed_items,
std::list<mojom::PromotedArticlePtr> promoted_articles;
std::list<mojom::DealPtr> deals;
std::hash<std::string> hasher;
base::flat_set<GURL> seen_articles;

for (auto& item : feed_items) {
if (!ShouldDisplayFeedItem(item, publishers, channels)) {
continue;
}
auto& metadata = MetadataFromFeedItem(item);
if (seen_articles.contains(metadata->url)) {
VLOG(2) << "Skipping " << metadata->url
<< " because we've already seen it.";
continue;
}

seen_articles.insert(metadata->url);
const auto& publisher = publishers->at(metadata->publisher_id);
// ShouldDisplayFeedItem should already have returned false
// if publishers doesn't have this publisher_id.
Expand Down
27 changes: 27 additions & 0 deletions components/brave_today/browser/feed_building_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -450,4 +450,31 @@ TEST_F(BraveNewsFeedBuildingTest, ChannelIsUsedWhenV2IsEnabled) {
EXPECT_FALSE(ShouldDisplayFeedItem(feed_item, &publisher_list, channels));
}

TEST_F(BraveNewsFeedBuildingTest, DuplicateItemsAreNotIncluded) {
// 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;

// Parse the feed items twice so we get two copies of everything.
ParseFeedItems(GetFeedJson(), &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);
ASSERT_EQ(feed.pages[0]->items.size(), 18u);
}

} // namespace brave_news
Loading