Merge pull request #16028 from brave/1.46.x-news-bug-fixes

[Uplift 1.46.x] Brave News bug fixes

browser/brave_news/brave_news_tab_helper.cc

@@ -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() {

chromium_src/ui/views/controls/button/md_text_button.cc

@@ -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),

components/brave_today/browser/brave_news_controller.cc

@@ -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);
@@ -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);
           },

components/brave_today/browser/brave_news_controller.h

@@ -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.

components/brave_today/browser/channels_controller.h

@@ -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,

components/brave_today/browser/direct_feed_controller.cc

@@ -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"
@@ -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));
 }
 
@@ -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;
@@ -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,
@@ -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,
@@ -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)));
           },
@@ -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;
@@ -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,

components/brave_today/browser/direct_feed_controller_unittest.cc

@@ -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>
@@ -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);
@@ -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());

components/brave_today/browser/feed_building.cc

@@ -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"
@@ -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 {
 
@@ -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;
 }
 
@@ -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.

components/brave_today/browser/feed_building_unittest.cc

@@ -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