Merge pull request #15820 from brave/bn-uplifts

[Uplift] [Brave News] Fix direct feeds & scrolling bug

browser/brave_news/BUILD.gn

@@ -9,11 +9,12 @@ source_set("brave_news") {
     "//brave/browser/profiles:util",
     "//brave/components/brave_today/browser",
     "//brave/components/brave_today/common",
-    "//brave/components/brave_today/common:common",
     "//brave/components/brave_today/common:mojom",
     "//chrome/browser:browser",
+    "//chrome/browser/favicon",
+    "//chrome/browser/profiles",
     "//chrome/browser/profiles:profile",
-    "//chrome/browser/profiles:profiles",
+    "//components/favicon/core",
     "//components/keyed_service/content",
     "//components/user_prefs",
     "//content/public/browser",

browser/brave_news/brave_news_controller_factory.cc

@@ -8,10 +8,13 @@
 #include "brave/browser/brave_ads/ads_service_factory.h"
 #include "brave/browser/profiles/profile_util.h"
 #include "brave/components/brave_today/browser/brave_news_controller.h"
+#include "chrome/browser/favicon/favicon_service_factory.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/profiles/incognito_helpers.h"
 #include "chrome/browser/profiles/profile.h"
+#include "components/favicon/core/favicon_service.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "components/keyed_service/core/service_access_type.h"
 #include "content/public/browser/browser_context.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 
@@ -49,6 +52,7 @@ BraveNewsControllerFactory::BraveNewsControllerFactory()
           "BraveNewsControllerFactory",
           BrowserContextDependencyManager::GetInstance()) {
   DependsOn(brave_ads::AdsServiceFactory::GetInstance());
+  DependsOn(FaviconServiceFactory::GetInstance());
   DependsOn(HistoryServiceFactory::GetInstance());
 }
 
@@ -63,11 +67,13 @@ KeyedService* BraveNewsControllerFactory::BuildServiceInstanceFor(
   if (!profile) {
     return nullptr;
   }
+  auto* favicon_service = FaviconServiceFactory::GetForProfile(
+      profile, ServiceAccessType::EXPLICIT_ACCESS);
   auto* ads_service = brave_ads::AdsServiceFactory::GetForProfile(profile);
   auto* history_service = HistoryServiceFactory::GetForProfile(
       profile, ServiceAccessType::EXPLICIT_ACCESS);
-  return new BraveNewsController(profile->GetPrefs(), ads_service,
-                                 history_service,
+  return new BraveNewsController(profile->GetPrefs(), favicon_service,
+                                 ads_service, history_service,
                                  profile->GetURLLoaderFactory());
 }
 

components/brave_new_tab_ui/api/brave_news/news.ts

@@ -171,4 +171,5 @@ class BraveNewsApi {
   }
 }
 
-export const api = new BraveNewsApi()
+export const api = new BraveNewsApi();
+(window as any).api = api

components/brave_new_tab_ui/components/default/braveToday/customize/Configure.tsx

@@ -23,6 +23,9 @@ const Grid = styled.div`
   width: 100%;
   height: 100%;
 
+  overflow: auto;
+  overscroll-behavior: none;
+
   display: grid;
   grid-template-columns: 250px auto;
   grid-template-rows: 64px 2px auto;

components/brave_new_tab_ui/components/default/braveToday/customize/SourcesListEntry.tsx

@@ -8,7 +8,7 @@ import styled from 'styled-components'
 import { getLocale } from '$web-common/locale'
 import Flex from '../../../Flex'
 import { useChannelSubscribed, usePublisher, usePublisherFollowed } from './Context'
-import { useLazyUnpaddedImageUrl } from '../useUnpaddedImageUrl'
+import { useLazyFavicon } from '../useUnpaddedImageUrl'
 import { getTranslatedChannelName } from './ChannelCard'
 
 interface Props {
@@ -61,11 +61,10 @@ const ChannelNameText = styled.span`
   font-weight: 600;
 `
 
-function FavIcon (props: { src?: string }) {
-  const { url, setElementRef } = useLazyUnpaddedImageUrl(props.src, {
+function FavIcon (props: { publisherId: string }) {
+  const { url, setElementRef } = useLazyFavicon(props.publisherId, {
     rootElement: document.getElementById('brave-news-configure'),
-    rootMargin: '0px 0px 100px 0px',
-    useCache: true
+    rootMargin: '200px 0px 200px 0px'
   })
   const [error, setError] = React.useState(false)
   return <FavIconContainer ref={setElementRef}>
@@ -79,7 +78,7 @@ export function FeedListEntry (props: Props) {
 
   return <Container direction="row" justify="space-between" align='center'>
     <Flex align='center' gap={8}>
-      <FavIcon src={publisher.faviconUrl?.url} />
+      <FavIcon publisherId={props.publisherId} />
       <Text>{publisher.publisherName}</Text>
     </Flex>
     <ToggleButton onClick={() => setFollowed(false)}>

components/brave_new_tab_ui/components/default/braveToday/customize/useSearch.ts

@@ -39,11 +39,16 @@ export default function useSearch (query: string) {
         p.feedSource?.url?.toLocaleLowerCase().includes(lowerQuery)))
     const results = { publishers, direct: [] as FeedSearchResultItem[] }
     for (const result of directResults) {
-      const publisherMatch = publishers.find(p => p.feedSource.url === result.feedUrl.url)
-      if (publisherMatch && !publishers.some(p => p.publisherId === publisherMatch.publisherId)) {
-        results.publishers.push(publisherMatch)
-      } else {
+      // If we have any publisher with this feed url, we should prefer showing
+      // that to creating a new direct feed.
+      // Note: We should only add the publisher to the list of publisher matches
+      // if it isn't already matching. The canonical case here is
+      // news.com (redirects to cnet.com), so we should show the cnet.com feed.
+      const publisherMatch = context.sortedPublishers.find(p => p.feedSource.url === result.feedUrl.url)
+      if (!publisherMatch) {
         results.direct.push(result)
+      } else if (!results.publishers.some(p => p.publisherId === publisherMatch.publisherId)) {
+        results.publishers.push(publisherMatch)
       }
     }
     return {

components/brave_new_tab_ui/components/default/braveToday/useUnpaddedImageUrl.ts

@@ -5,6 +5,7 @@
 
 import * as React from 'react'
 import getBraveNewsController from '../../../api/brave_news'
+import { useVisible, VisibleOptions } from '$web-common/useVisible'
 
 interface Options {
   rootElement?: HTMLElement | null
@@ -16,6 +17,46 @@ interface Options {
 }
 
 const cache: { [url: string]: string } = {}
+function useFavicon (publisherId: string | undefined) {
+  const [url, setUrl] = React.useState<string>(cache[publisherId!])
+  React.useEffect(() => {
+    if (!publisherId) {
+      return
+    }
+
+    if (cache[publisherId]) {
+      setUrl(cache[publisherId])
+      return
+    }
+
+    let cancelled = false
+    getBraveNewsController().getFavIconData(publisherId).then(({ imageData }) => {
+      if (cancelled) return
+      if (!imageData) return
+
+      const blob = new Blob([new Uint8Array(imageData)], { type: 'image/*' })
+      const url = URL.createObjectURL(blob)
+      cache[publisherId] = url
+      setUrl(url)
+    })
+
+    return () => {
+      cancelled = true
+    }
+  }, [publisherId])
+
+  return url
+}
+
+export function useLazyFavicon (publisherId: string, options: VisibleOptions) {
+  const { visible, setElementRef } = useVisible(options)
+  const url = useFavicon(visible ? publisherId : undefined)
+  return {
+    url,
+    setElementRef
+  }
+}
+
 export function useUnpaddedImageUrl (paddedUrl: string | undefined, onLoaded?: () => any, useCache?: boolean) {
   const [unpaddedUrl, setUnpaddedUrl] = React.useState('')
 
@@ -67,28 +108,7 @@ export function useUnpaddedImageUrl (paddedUrl: string | undefined, onLoaded?: (
 }
 
 export function useLazyUnpaddedImageUrl (paddedUrl: string | undefined, options: Options) {
-  const [visible, setVisible] = React.useState(false)
-  const [elementRef, setElementRef] = React.useState<HTMLElement | null>(null)
-
-  React.useEffect(() => {
-    if (!elementRef) return
-
-    const observer = new IntersectionObserver(([intersectionInfo]) => {
-      if (!intersectionInfo.isIntersecting) {
-        return
-      }
-      setVisible(true)
-    }, {
-      root: options.rootElement,
-      rootMargin: options.rootMargin,
-      threshold: options.threshold
-    })
-
-  observer.observe(elementRef)
-    return () => {
-      observer.disconnect()
-    }
-  }, [options.rootElement, elementRef])
+  const { visible, setElementRef } = useVisible(options)
 
   return {
     url: useUnpaddedImageUrl(visible ? paddedUrl : undefined, options.onLoaded, options.useCache) || cache[paddedUrl!],

components/brave_today/browser/BUILD.gn

@@ -45,6 +45,8 @@ static_library("browser") {
     "//brave/components/l10n/common",
     "//brave/components/p3a_utils",
     "//brave/components/time_period_storage",
+    "//components/favicon/core:core",
+    "//components/favicon_base:favicon_base",
     "//components/feed:buildflags",
     "//components/history/core/browser",
     "//components/keyed_service/core",

components/brave_today/browser/brave_news_controller.cc

@@ -5,6 +5,7 @@
 
 #include "brave/components/brave_today/browser/brave_news_controller.h"
 
+#include <algorithm>
 #include <cmath>
 #include <memory>
 #include <string>
@@ -37,6 +38,8 @@
 #include "brave/components/brave_today/common/features.h"
 #include "brave/components/brave_today/common/pref_names.h"
 #include "brave/components/l10n/common/locale_util.h"
+#include "components/favicon/core/favicon_service.h"
+#include "components/favicon_base/favicon_types.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/scoped_user_pref_update.h"
@@ -45,6 +48,11 @@
 #include "third_party/abseil-cpp/absl/types/optional.h"
 
 namespace brave_news {
+namespace {
+// The favicon size we desire. The favicons are rendered at 24x24 pixels but
+// they look quite a bit nicer if we get a 48x48 pixel icon and downscale it.
+constexpr uint32_t kDesiredFaviconSizePixels = 48;
+}  // namespace
 
 bool IsPublisherEnabled(const mojom::Publisher* publisher) {
   if (!publisher)
@@ -77,10 +85,12 @@ void BraveNewsController::RegisterProfilePrefs(PrefRegistrySimple* registry) {
 
 BraveNewsController::BraveNewsController(
     PrefService* prefs,
+    favicon::FaviconService* favicon_service,
     brave_ads::AdsService* ads_service,
     history::HistoryService* history_service,
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
     : prefs_(prefs),
+      favicon_service_(favicon_service),
       ads_service_(ads_service),
       api_request_helper_(GetNetworkTrafficAnnotationTag(), url_loader_factory),
       private_cdn_request_helper_(GetNetworkTrafficAnnotationTag(),
@@ -302,6 +312,54 @@ void BraveNewsController::GetImageData(const GURL& padded_image_url,
           std::move(callback), is_padded));
 }
 
+void BraveNewsController::GetFavIconData(const std::string& publisher_id,
+                                         GetFavIconDataCallback callback) {
+  GetPublishers(base::BindOnce(
+      [](BraveNewsController* controller, const std::string& publisher_id,
+         GetFavIconDataCallback callback, Publishers publishers) {
+        // If the publisher doesn't exist, there's nothing we can do.
+        const auto& it = publishers.find(publisher_id);
+        if (it == publishers.end()) {
+          std::move(callback).Run(absl::nullopt);
+          return;
+        }
+
+        // If we have a FavIcon url, use that.
+        const auto& publisher = it->second;
+        if (publisher->favicon_url) {
+          controller->GetImageData(publisher->favicon_url.value(),
+                                   std::move(callback));
+          return;
+        }
+
+        auto source_url = publisher->site_url.is_valid()
+                              ? publisher->site_url
+                              : publisher->feed_source;
+        favicon_base::IconTypeSet icon_types{
+            favicon_base::IconType::kFavicon,
+            favicon_base::IconType::kTouchIcon};
+        controller->favicon_service_->GetRawFaviconForPageURL(
+            source_url, icon_types, kDesiredFaviconSizePixels, true,
+            base::BindOnce(
+                [](GetFavIconDataCallback callback,
+                   const favicon_base::FaviconRawBitmapResult& result) {
+                  if (!result.is_valid()) {
+                    std::move(callback).Run(absl::nullopt);
+                    return;
+                  }
+
+                  auto bytes = result.bitmap_data;
+                  std::vector<uint8_t> bytes_vec(
+                      bytes->front_as<uint8_t>(),
+                      bytes->front_as<uint8_t>() + bytes->size());
+                  std::move(callback).Run(std::move(bytes_vec));
+                },
+                std::move(callback)),
+            &controller->task_tracker_);
+      },
+      base::Unretained(this), publisher_id, std::move(callback)));
+}
+
 void BraveNewsController::SetPublisherPref(const std::string& publisher_id,
                                            mojom::UserEnabled new_status) {
   VLOG(1) << "set publisher pref: " << new_status;

components/brave_today/browser/brave_news_controller.h

@@ -6,12 +6,14 @@
 #ifndef BRAVE_COMPONENTS_BRAVE_TODAY_BROWSER_BRAVE_NEWS_CONTROLLER_H_
 #define BRAVE_COMPONENTS_BRAVE_TODAY_BROWSER_BRAVE_NEWS_CONTROLLER_H_
 
+#include <cstddef>
 #include <memory>
 #include <string>
 
 #include "base/callback_forward.h"
 #include "base/containers/flat_map.h"
 #include "base/memory/raw_ptr.h"
+#include "base/task/cancelable_task_tracker.h"
 #include "base/timer/timer.h"
 #include "brave/components/api_request_helper/api_request_helper.h"
 #include "brave/components/brave_private_cdn/private_cdn_request_helper.h"
@@ -37,6 +39,10 @@ namespace brave_ads {
 class AdsService;
 }  // namespace brave_ads
 
+namespace favicon {
+class FaviconService;
+}
+
 namespace history {
 class HistoryService;
 }  // namespace history
@@ -56,6 +62,7 @@ class BraveNewsController : public KeyedService,
 
   BraveNewsController(
       PrefService* prefs,
+      favicon::FaviconService* favicon_service,
       brave_ads::AdsService* ads_service,
       history::HistoryService* history_service,
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory);
@@ -92,6 +99,8 @@ class BraveNewsController : public KeyedService,
   void RemoveDirectFeed(const std::string& publisher_id) override;
   void GetImageData(const GURL& padded_image_url,
                     GetImageDataCallback callback) override;
+  void GetFavIconData(const std::string& publisher_id,
+                      GetFavIconDataCallback callback) override;
   void SetPublisherPref(const std::string& publisher_id,
                         mojom::UserEnabled new_status) override;
   void ClearPrefs() override;
@@ -122,6 +131,7 @@ class BraveNewsController : public KeyedService,
   void Prefetch();
 
   raw_ptr<PrefService> prefs_ = nullptr;
+  raw_ptr<favicon::FaviconService> favicon_service_ = nullptr;
   raw_ptr<brave_ads::AdsService> ads_service_ = nullptr;
   api_request_helper::APIRequestHelper api_request_helper_;
   brave_private_cdn::PrivateCDNRequestHelper private_cdn_request_helper_;
@@ -137,6 +147,7 @@ class BraveNewsController : public KeyedService,
   base::OneShotTimer timer_prefetch_;
   base::RepeatingTimer timer_feed_update_;
   base::RepeatingTimer timer_publishers_update_;
+  base::CancelableTaskTracker task_tracker_;
 
   mojo::ReceiverSet<mojom::BraveNewsController> receivers_;
   base::WeakPtrFactory<BraveNewsController> weak_ptr_factory_;